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

add bmp option #4537

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eldadcool
Copy link

This PR is a suggestion for a fix to #4536.

All I did is added the BMP_STRING format to the relevant type

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.69%. Comparing base (8e08cbf) to head (eb78e88).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4537      +/-   ##
==========================================
- Coverage   81.62%   78.69%   -2.94%     
==========================================
  Files         358      333      -25     
  Lines       85652    80454    -5198     
==========================================
- Hits        69915    63313    -6602     
- Misses      15737    17141    +1404     
Files with missing lines Coverage Δ
scapy/layers/x509.py 97.79% <ø> (-0.01%) ⬇️

... and 284 files with indirect coverage changes

@guedou
Copy link
Member

guedou commented Sep 21, 2024

Thanks for the fix! I guess that a test should also be added, using a pattern similar to https://github.com/secdev/scapy/blob/master/test/scapy/layers/x509.uts#L53

I managed to decrease the size of the certificate to 480 bytes using the following script:

from scapy.all import *

load_layer("tls")

c = Cert("example_cert.pem")

del(c.tbsCertificate.serialNumber)
del(c.tbsCertificate.signature)
del(c.tbsCertificate.subject)
del(c.tbsCertificate.validity)
del(c.tbsCertificate.subjectPublicKeyInfo)
del(c.tbsCertificate.extensions)
del(c.x509Cert.signatureAlgorithm)
del(c.x509Cert.signatureValue)

open("example_cert.new.der", "wb").write(raw(c.x509Cert))
c = Cert("example_cert.new.pem")
c.x509Cert.show()

@eldadcool
Copy link
Author

eldadcool commented Sep 24, 2024

Thanks for the fix! I guess that a test should also be added, using a pattern similar to https://github.com/secdev/scapy/blob/master/test/scapy/layers/x509.uts#L53

I tried to add a test but encounter a few difficulties:

  1. How can I run the tests? just running pytest does not work and I did not found anything in the docs... (Maybe it's because I'm in windows?)
  2. I can write a very explicit check like:
cert_with_bmp_string = base64.b64decode('MIIHKjCCB...Or8iY=')
c = X509_Cert(cert_with_bmp_string)
bmp_field_value = str(c.tbsCertificate.issuer[7].rdn[0].value.val, "utf-16be")
assert bmp_field_value == '[email protected]'

But I really don't like it. So I tried to use the function get_issuer_str but the output was a mess because of the encoding:

c.tbsCertificate.get_issuer_str()
'/C=US/ST=CA/L=LG/O=Websense, Inc./OU=Websense Endpoint/CN=Websense Public Primary Certificate Authority/description=\x001\x002\x004\x006\x001\x008\x003\x005\x001\x004\x00E\x00P\x00@\x00w\x00e\x00b\x00s\x00e\x00n\x00s\x00e\x00.\x00c\x00o\x00m/[email protected]'

This might be something that require deeper modification of the code.
This happens because any non utf8 encoding is replaced with the backslash notation using the decode function

x.decode(errors="backslashreplace")

So, in calculation:

  • I think the decoding of ASN1 strings should be done with a method and every type of string can replace the encoding method.
  • After that fix I suggest writing the test as follow:
cert_with_bmp_string = base64.b64decode('MIIHKjCCB...Or8iY=')
c = X509_Cert(cert_with_bmp_string)
assert '[email protected]' in c.tbsCertificate.get_issuer_str()

@guedou, What do you think of my suggestion? should I include it in this PR?

@guedou
Copy link
Member

guedou commented Sep 25, 2024

You can test Scapy with UTScapy as described in https://scapy.readthedocs.io/en/latest/development.html#testing-with-utscapy. It is simpler to launch the tests using tox (see https://scapy.readthedocs.io/en/latest/development.html#using-tox-to-test-scapy) as it will take care of many things for you.

I think that your first test is quite fine, and that's what we usually do.

@eldadcool eldadcool force-pushed the add-bmp-string-option-to-directory-string branch from ab5f9a0 to eb78e88 Compare November 5, 2024 10:30
@eldadcool
Copy link
Author

eldadcool commented Nov 5, 2024

test was added and rebased on the updated master

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.

4 participants