scapy icon indicating copy to clipboard operation
scapy copied to clipboard

refactor RC2 support to work in an upcoming cryptography release

Open reaperhulk opened this issue 1 year ago • 19 comments

This PR uses the new unreleased (and undocumented) RC2 support in cryptography (https://github.com/pyca/cryptography/pull/10407), but retains the fallbacks for older versions so that scapy remains compatible. In the next release of cryptography we are oxidizing the cipher layer so the non-public APIs currently in use will cease functioning.

fixes https://github.com/secdev/scapy/issues/4237

reaperhulk avatar Feb 17 '24 03:02 reaperhulk

Codecov Report

Merging #4285 (a6811b2) into master (dedddd9) will decrease coverage by 0.02%. The diff coverage is 90.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4285      +/-   ##
==========================================
- Coverage   81.99%   81.98%   -0.02%     
==========================================
  Files         349      349              
  Lines       81889    81897       +8     
==========================================
- Hits        67148    67140       -8     
- Misses      14741    14757      +16     
Files Coverage Δ
scapy/layers/tls/crypto/cipher_block.py 95.68% <90.90%> (-1.27%) :arrow_down:

... and 4 files with indirect coverage changes

codecov[bot] avatar Feb 17 '24 03:02 codecov[bot]

Looks like the most recent changes are causing failures in our CI right now (https://github.com/secdev/scapy/commit/16e13720fcb29b6452bd9422973375feb2d313ee#diff-750e485bcc229105845db52d66b35efca6075324730a92e7b8c921e75b741ccdL3) and also have significantly slowed down testing. Anything we can do there? 😄

reaperhulk avatar Feb 18 '24 04:02 reaperhulk

Hi. Sorry about that, do you have an example of a failing runner?

gpotter2 avatar Feb 18 '24 12:02 gpotter2

@gpotter2 https://github.com/pyca/cryptography/actions/runs/7946618525/job/21694556742?pr=10414 -- it times out after 15 minutes

alex avatar Feb 18 '24 13:02 alex

@alex Got it, thanks. Could you retry with https://github.com/secdev/scapy/pull/4288 merged? If that still doesn't do it I'll investigate more & disable the test.

gpotter2 avatar Feb 18 '24 13:02 gpotter2

Good news, they now run much faster (no timeout!).

Bad news, they seem to fail: https://github.com/pyca/cryptography/actions/runs/7946618525/job/21700730313?pr=10414

alex avatar Feb 18 '24 14:02 alex

Appears to be failing with

 Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/threading.py", line 1073, in _bootstrap_inner
    self.run()
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/threading.py", line 1010, in run
    self._target(*self._args, **self._kwargs)
  File "<input>", line 29, in run_tls_test_server
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/automaton.py", line 1497, in run
    raise value
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/automaton.py", line 1292, in _do_control
    state = next(iterator)
            ^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/automaton.py", line 1350, in _do_iter
    self._run_condition(cond, *state_output)
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/automaton.py", line 1214, in _run_condition
    cond(self, *args, **kargs)
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/automaton_srv.py", line 369, in should_send_ServerFlight1
    self.flush_records()
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/automaton.py", line 265, in flush_records
    s = b"".join(p.raw_stateful() for p in self.buffer_out)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/automaton.py", line 265, in <genexpr>
    s = b"".join(p.raw_stateful() for p in self.buffer_out)
                 ^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/session.py", line 1047, in raw_stateful
    return super(_GenericTLSSessionInheritance, self).__bytes__()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/packet.py", line 599, in __bytes__
    return self.build()
           ^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/packet.py", line 758, in build
    p = self.do_build()
        ^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/packet.py", line 738, in do_build
    pkt = self.self_build()
          ^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/packet.py", line 717, in self_build
    raise ex
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/packet.py", line 708, in self_build
    p = f.addfield(self, p, val)
        ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/record.py", line 206, in addfield
    res += self.i2m(pkt, p)
           ^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/record.py", line 190, in i2m
    cur = p.raw_stateful()
          ^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/session.py", line 1047, in raw_stateful
    return super(_GenericTLSSessionInheritance, self).__bytes__()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/packet.py", line 599, in __bytes__
    return self.build()
           ^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/handshake.py", line 1044, in build
    cls.fill_missing()
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/config.py", line 1161, in func_in
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/keyexchange.py", line 662, in fill_missing
    k.fill_and_store(modulusLen=512)
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/config.py", line 1161, in func_in
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cryptography/cryptography/scapy/scapy/layers/tls/cert.py", line 473, in fill_and_store
    self.key = rsa.generate_private_key(public_exponent=pubExp,
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/cryptography/hazmat/primitives/asymmetric/rsa.py", line 142, in generate_private_key
    _verify_rsa_parameters(public_exponent, key_size)
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/cryptography/hazmat/primitives/asymmetric/rsa.py", line 154, in _verify_rsa_parameters
    raise ValueError("key_size must be at least 1024-bits.")
ValueError: While dissecting field 'msg': key_size must be at least 1024-bits.

(then subsequent failures are due to the first one)

gpotter2 avatar Feb 18 '24 14:02 gpotter2

On main we've raised the minimum key size to generate a new RSA key to 1024-bit (previously it was 512-bit).

Can the key size here be raised to 1024-bit? If not, you can still load an existing, smaller, key.

alex avatar Feb 18 '24 14:02 alex

This test is using TLS_RSA_EXPORT_WITH_RC4_40_MD5 which is a cipher part of the "export restrictions" era. See https://datatracker.ietf.org/doc/html/rfc2246#autoid-66. So I'm pretty sure we do want a key no bigger than 512bits for those.

I'll take a look of how we can work around this, probably by trying to generate the key ourselves or something.

gpotter2 avatar Feb 18 '24 14:02 gpotter2

Hmm, in that case, I think the best solution would be to use a pre-generated (hardcoded) key.

alex avatar Feb 18 '24 14:02 alex

It bothers me to hardcode the key. I'd prefer as much as possible to keep the SSLv2-TLS1.0 implementation in a working, 'per-spec' state, as nowadays there aren't many implementations available that still support those old algorithms (yet they're still encountered in the wild).

Do you mind if I 'fix' this by adding a hack that calls rust_openssl.rsa.generate_private_key(65537, 512) directly?

gpotter2 avatar Feb 18 '24 14:02 gpotter2

I can't promise that will work forever (and it won't work on older pyca/cryptography), but for now it will work, and if we do change it, our CI will catch it.

alex avatar Feb 18 '24 14:02 alex

Okay, that works. See https://github.com/secdev/scapy/pull/4290. Tested with cryptography's main and it seems to be working, so I'll merge it once the tests pass.

gpotter2 avatar Feb 18 '24 14:02 gpotter2

Ooops, I don't think this should have been closed :-)

alex avatar Feb 18 '24 15:02 alex

@gpotter2 Fix pushed (and also rebased against master once more) 😄

reaperhulk avatar Feb 18 '24 16:02 reaperhulk

The RC2-40 test fails for me locally on our main branch so it appears some more investigation is required on my end.

reaperhulk avatar Feb 18 '24 16:02 reaperhulk

Okay, thanks.

gpotter2 avatar Feb 18 '24 16:02 gpotter2

Should be fixed now 😄

reaperhulk avatar Feb 18 '24 17:02 reaperhulk

Test failure is in send() and sniff() https://github.com/secdev/scapy/actions/runs/7950566439/job/21703035999?pr=4285#step:5:688

This doesn't look related to the current PR as best I can tell.

reaperhulk avatar Feb 18 '24 17:02 reaperhulk