Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor RC2 support to work in an upcoming cryptography release #4285

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

reaperhulk
Copy link
Contributor

@reaperhulk reaperhulk commented Feb 17, 2024

This PR uses the new unreleased (and undocumented) RC2 support in cryptography (pyca/cryptography#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 #4237

Copy link

codecov bot commented Feb 17, 2024

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%) ⬇️

... and 4 files with indirect coverage changes

Copy link
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the work and PR !

scapy/layers/tls/crypto/cipher_block.py Outdated Show resolved Hide resolved
@gpotter2 gpotter2 mentioned this pull request Feb 17, 2024
@reaperhulk reaperhulk force-pushed the rc2-oh-no branch 2 times, most recently from 3163ff4 to b5a68ed Compare February 17, 2024 18:32
@reaperhulk
Copy link
Contributor Author

Looks like the most recent changes are causing failures in our CI right now (16e1372#diff-750e485bcc229105845db52d66b35efca6075324730a92e7b8c921e75b741ccdL3) and also have significantly slowed down testing. Anything we can do there? 😄

@gpotter2
Copy link
Member

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

@alex
Copy link
Contributor

alex commented Feb 18, 2024

@gpotter2
Copy link
Member

@alex Got it, thanks. Could you retry with #4288 merged? If that still doesn't do it I'll investigate more & disable the test.

@alex
Copy link
Contributor

alex commented Feb 18, 2024

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

@gpotter2
Copy link
Member

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)

@alex
Copy link
Contributor

alex commented Feb 18, 2024

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.

@gpotter2
Copy link
Member

gpotter2 commented Feb 18, 2024

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.

@alex
Copy link
Contributor

alex commented Feb 18, 2024

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

@gpotter2
Copy link
Member

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?

@alex
Copy link
Contributor

alex commented Feb 18, 2024

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.

@gpotter2
Copy link
Member

Okay, that works. See #4290. Tested with cryptography's main and it seems to be working, so I'll merge it once the tests pass.

@gpotter2 gpotter2 reopened this Feb 18, 2024
@alex
Copy link
Contributor

alex commented Feb 18, 2024

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

@gpotter2 gpotter2 added this to the 2.6.0 milestone Feb 18, 2024
Copy link
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good but currently doesn't work against cryptography's main, because of the GetCipherByName import at the beggining of the file. You likely want to move it in the < 43.0 section

@reaperhulk
Copy link
Contributor Author

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

@reaperhulk
Copy link
Contributor Author

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

@gpotter2
Copy link
Member

Okay, thanks.

@reaperhulk
Copy link
Contributor Author

Should be fixed now 😄

@reaperhulk
Copy link
Contributor Author

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.

Copy link
Member

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR !

@gpotter2 gpotter2 merged commit 2267cc5 into secdev:master Feb 18, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tls/crypto /cipher_block.py relies on cryptography internal details that are going away eventually
3 participants