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

hash algorithms used for signatures #128

Merged
merged 2 commits into from
Jan 21, 2020
Merged

hash algorithms used for signatures #128

merged 2 commits into from
Jan 21, 2020

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Jan 20, 2020

fixes #123

for @psafont I added val signature_algorithm : t -> ([ `RSA | `ECDSA ] * Nocrypto.Hash.hash) option to the Certificate, Signing_request, and CRL modules. Does this suite you well?

The (chain) validation functions now receive an optional hash_whitelist argument, which the signature algorithm must match. The default is any SHA-2 algorithm (SHA256/SHA384/SHA512, not SHA224 - seems to be rarely used, I could not see a reason to include it). The default for valid_ca{,s} is any hash algorithm (following what others are doing).

/cc @emillon @paurkedal for a quick round of review -- this certainly changes the behaviour of x509 & tls -- in a good way forward (rejecting weak algorithms by default).

lib/crl.ml Show resolved Hide resolved
Error (`Revoked cert)
else begin
Ok () >>= fun () ->
signs hash pathlen issuer cert >>= fun () ->
Copy link

Choose a reason for hiding this comment

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

I'm not sure how in what order the checks are done, but it seems that if the hash is not trusted, you don't even need to walk the validation chain (or to do any kind of crypto). Not sure that easier to implement this way, though.

Copy link
Member Author

@hannesm hannesm Jan 21, 2020

Choose a reason for hiding this comment

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

let me briefly describe the implementation: the external entry points into validation are verify_chain and verify_chain_of_trust, where the latter is used by Authenticator.chain_of_trust.

Either verify_ function calls verify_single_chain as workhorse (with the chain in order server_certificate :: intermediate1 :: intermediate2 :: last_intermediate, the server certificate is already validated (in respect to is_server_cert_valid)).
Now, verify_single_chain is recursive, if only a single certificate is left, this is checked against the trust anchors. If multiple certificates are in the chain (cert :: issuer :: certs), the issuer is checked to be valid (validity period, CA extensions -- is_cert_valid), and signs is called subsequently with the path length (increasing on each recursive call), and the issuer and cert as arguments.

signs does four checks:

  • does the issuer of the cert match the subject of the issuer cert?
  • if the Authority_key_id and Subject_key_id are present in the certificates, do they match?
  • validate signature (this does the actual work)
  • is the path length sufficient (if the BasicConstraints extension is present in the issuer)

The validate_signature function purpose is as follows (with the following pieces: a certificate consists of three things: the to-be-signed-certificate data (public key, name, signature_algorithm, ..), the signature_algorithm (unsigned), and the signature itself)

  • (a) extracts the RSA public key from the issuing certificate
  • (b) decrypts the signature (Rsa PKCS1) -- this is the computationally intensive work, asymmetric crypto operation
  • (c) decodes the output from above as PKCS1 digest info, resulting in a pair of sig_alg and hash value
  • (d) compares sig_alg with signature_algorithm
  • (e) computes the hash of raw tbs-cert -- as well computationally intensive work (hashing)
  • (f) and compares the computed hash against the hash decoded from the signature value

now, this PR added "check against whitelist" between (c) and (d); I revised in 9375e07 to do the check between (a) and (b).

EDIT: fundamentally we have two signature_algo (one signed within the TBS, one part of the signature) that must be equal, and now in addition this signature_algo must be in the provided whitelist. The order of checks does not matter, as long as both equality and membership is ensured.

it seems that if the hash is not trusted, you don't even need to walk the validation chain

the implemented functionality walks the chain once, and does pairwise validation. I'm not keen on any preprocessing (as "check for the hash algorithms used in the signatures"), since that'd make the code less readable IMHO (it is complex since some properties need to be checked, as well as potentially existing extensions, the absence of other extensions, etc.). RFC 5280 contains a detailed description of path validation, implemented here (apart from missing things such as name constraints #65).

I'm not sure how in what order the checks are done

I hope the above explains it better to you. For me the above is rather clear from the code, do you have any suggestions on how to improve the structure (or naming?) to make the code easier to follow?

Copy link

Choose a reason for hiding this comment

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

that all makes sense, thanks for the writeup!

@psafont
Copy link
Contributor

psafont commented Jan 20, 2020

This will work wonders, thank you!

…ist of

hash algorithms used for certificate validation. Default is SHA-2
(without SHA-224) for all functions taking this argument, apart from
valid_ca{,s} which defaults to all hash algorithms (including MD5 and SHA1).
@hannesm hannesm merged commit b3787a6 into mirleft:master Jan 21, 2020
@hannesm hannesm deleted the sigalg branch January 21, 2020 19:48
lib/x509.mli Show resolved Hide resolved
lib/validation.ml Show resolved Hide resolved
hannesm added a commit to hannesm/opam-repository that referenced this pull request Jan 22, 2020
CHANGES:

* BREAKING add a whitelist of hash algorithms used for signatures. The default
  whitelist is the SHA-2 family (without SHA-224), Validation.valid_ca{,s} use
  all algorithms as default
  reported by @emillon in mirleft/ocaml-x509#123, fixed in mirleft/ocaml-x509#128
* BREAKING Certificate.hostnames and Signing_request.hostnames (new) return a
  set of [`Wildcard|`Strict] * [`host] Domain_name.t (Certificate.Host_set.t)
  reported by @mmaker in mirleft/ocaml-x509#88, fixed in mirleft/ocaml-x509#127
* BREAKING mirleft/ocaml-x509#127 Signing_request.sign returns a result type now, an error is
  returned if the signing request was not properly signed
* BREAKING mirleft/ocaml-x509#127 Validation.{verify_chain_of_trust, trust_key_fingerprint,
  trust_cert_fingerptint} and the type Authenticator.t changed, no longer use
  of a Certificate.host, but instead a [`host] Domain_name.t (previously, it was
  a pair)
* BUGFIX support AlgorithmIdentifier of RSA signature algorithms with parameter
  not present
  reported by @Ulrar in mirleft/ocaml-x509#108, fixed in mirleft/ocaml-x509#129
* BUGFIX mirleft/ocaml-x509#127 preserve a signed signing request (Country in a DN sometimes uses
  a non-utf8 string encoding)
* remove deprecation from Validation.trust_cert_fingerprint and
  Authenticator.server_cert_fingerprint
  requested by @mben-romdhane in mirleft/ocaml-x509#125, fixed in mirleft/ocaml-x509#126
* Certificate.signature_algorithm, CRL.signature_algorithm, and
  Signing_request.signature_algorithm are now provided, returning a
  ([`RSA|`ECDSA] * Nocrypto.Hash.hash) option
  requested by @psafont in mirleft/ocaml-x509#123, fixed in mirleft/ocaml-x509#128
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Certificate verification allows dangerous algorithms
4 participants