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

tls module serial numbers should have at least 20 bits of entropy #16744

Closed
dstufft opened this issue Oct 19, 2014 · 4 comments
Closed

tls module serial numbers should have at least 20 bits of entropy #16744

dstufft opened this issue Oct 19, 2014 · 4 comments
Assignees
Labels
Bug broken, incorrect, or confusing behavior Execution-Module help-wanted Community help is needed to resolve this P2 Priority 2 Platform Relates to OS, containers, platform-based utilities like FS, system based apps severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@dstufft
Copy link

dstufft commented Oct 19, 2014

Looking at the salt/modules/tls.py code I see that the _new_serial function creates a serial number based off a md5sum of ca_name, CN, and the time. This should be modified to have atleast 20 bits of entropy, but really the whole thing (or most of it) should probably just be random. This could be as simple as doing binascii.hexlify(os.urandom(20)).

The rationale is that one of the ways to defeat preimage attacks on certificate signing is by introducing entropy into the certificate payload. While an md5(ca_name + stuff + time) is unlikely to be exploited it would be considered best practice to provide as much entropy as possible in the serial. For those who are mega paranoid about fantastically improbable collisions you can do a smaller chunk from os.urandom and then add microtime to it and hexlify that payload.

In the guidelines for public CA's (https://cabforum.org/wp-content/uploads/Baseline_Requirements_V1_1_9.pdf section 9.6) it says:

CAs SHOULD generate non-sequential Certificate serial numbers that exhibit at least 20 bits of entropy

@cachedout
Copy link
Contributor

Wow, amazing catch! Thank you very much for reviewing the code so well. We will sure and get this cleaned up.

@cachedout cachedout added Bug broken, incorrect, or confusing behavior Execution-Module help-wanted Community help is needed to resolve this severity-low 4th level, cosemtic problems, work around exists labels Oct 20, 2014
@cachedout cachedout added this to the Approved milestone Oct 20, 2014
@dstufft
Copy link
Author

dstufft commented Oct 20, 2014

If you tell me which branch to make the PR against and whether saltstack is "mega paranoid about fantastically improbable collisions" I can make a PR for this.

@cachedout
Copy link
Contributor

We'll happily accept a PR against the develop branch for whatever level of paranoia you deem appropriate. :]

@cachedout
Copy link
Contributor

@dstufft Did you still want to write a PR for this? Just checking in. :]

@jfindlay jfindlay added the Platform Relates to OS, containers, platform-based utilities like FS, system based apps label May 26, 2015
@basepi basepi added severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around P2 Priority 2 and removed severity-low 4th level, cosemtic problems, work around exists labels Jul 14, 2015
@cro cro self-assigned this Dec 3, 2015
@cro cro added the TEAM RIoT label Dec 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Execution-Module help-wanted Community help is needed to resolve this P2 Priority 2 Platform Relates to OS, containers, platform-based utilities like FS, system based apps severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

No branches or pull requests

5 participants