Closed Bug 1179338 (flexible-certverify) Opened 9 years ago Closed 8 years ago

Don't require that the signature method for certificate verify is identical with PRF, support alternatives

Categories

(NSS :: Libraries, defect)

Product:
NSS
Component:
Libraries
Version:
3.19
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: hkario, Assigned: KaiE)

References

Details

Attachments

(1 file, 11 obsolete files)

Patch v17
63.27 KB, patch
KaiE
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150603135725

Steps to reproduce:

Use NSS client or server with mandatory certificate verification in TLS.


Actual results:

Client is able to handle only SHA-1 and SHA-256 signatures on the Certificate Verify.
Server is asking only for SHA-256 signatures in Certificate Request message.


Expected results:

All SHA-2 hashes should be supported on client side: SHA-224, SHA-384 and SHA-512.
Server should allow the client to use any hash, SHA-1, SHA-224, SHA-384 or SHA-512, not only SHA-256.
Philosophically: Does SHA-224 even make sense? Microsoft have never implemented it, nor am I aware of anything mainstream that does. It's not something we'd ever enable for Chrome, primarily on the basis of interop reasons alone.

Curious whether or not NSS should implement it, especially if Firefox decides to follow the same, but my preference would be "No"
the sha-224 is recommended by NIST for signatures using 2048 bit RSA and DSA keys.

But then asking for SHA224+ECDSA is pretty useless given the most commonly used curves.
Recommended or not, it will never work on Windows, so why would anyone practically use it, and what would ever be the benefit of practically supporting it? It seems more harmful than helpful to support some esoteric option, and then have to explain to people that 99.999% of the time they can't use it w/o causing interoperability issues.

(The .001% is when you're not on the Internet and don't have any Windows users, and that's just... largely silly, especially when the alternative is just SHA-256 or SHA-384 if you're paranoid about extensions)
"99.999%" people[citation needed] don't use client certificates in web browsers

does that mean we should just remove support for client certificates in NSS?

no, it does not

then what Microsoft does or what Firefox and Chromium consider interesting is secondary to having implementation of that feature in NSS
You're mistaking interoperability for use. But more importantly, it should be an explicit non-goal of NSS to support something "just because" someone wrote an IETF I-D. That's precisely how Heartbleed happened for OpenSSL, that's precisely what s2n is trying to avoid, that's precisely what ocaml-tls is trying to avoid.

I'm just pointing out that the two largest users of NSS, by any reasonable margin, generally have a strong desire to not introduce things just because. Both Mozilla (AIUI) and Chromium have adopted policies regarding making sure that random features aren't just introduced to the Web Platform that present interoperability risks (see http://www.chromium.org/blink#new-features for a broader discussion), and it's entirely reasonable within NSS to examine the ecosystem holistically and ask "Just because there is a spec for it, would we be helping or harming the ecosystem in supporting"

A decision for SHA-224 is knowingly introducing a sharp-edge of interoperability concerns with a variety of TLS clients. If there's justifiable reasons and active interest, then maybe that sharp edge is worth it, and 'other implementations' are wrong. But that doesn't seem to be the case here, nor does there seem to be an active reason to support it other than 'just because', and that's more detrimental than beneficial.

At best, this should be something off by default (on the basis of interoperability alone), and once you go down that route, there's a question of whether or not it should be done in the first place. Who would use it - NIST recommendations not-withstanding?
I already said, this is not a proposal for Firefox or Chrome.
And my previously reply was precisely why it's concerning for *NSS* as a project and as a library.

The issues I described in my previous mail were extensively about libraries like NSS (GnuTLS, TLSLite, OpenSSL) and the approach to TLS features.

I want to be wholly explicit here: Not only do I think SHA-224 support doesn't belong in Firefox and Chrome, I don't believe it belongs in NSS, period. And, more broadly speaking, I think NSS should be careful about what features / things that have specs it implements.

Just making sure it's clear exactly what my objection is, because I fear you're misunderstanding and thinking my concerns as being Chrome/FF specific, which they're most definitely not.
I don't understand your hostility towards SHA-224, but yes, it's most likely not necessary.

When implementation of all the other hashes is present, adding it will likely amount to just few (likely less than 4) lines of simple code, so I really don't think the argument of complexity and likeliness of bugs introduced by adding it applies...
I'm not at against adding SHA-224. It was a late addition because NIST decided they needed it, but I think it good to chime in on the principle that Ryan is trying to convey:

We have to balance the ciphers we support with the potential for future attacks. Adding SHA-224 means that if we run into an attack against SHA-224, we suddenly have opened up ourselves to that attack. The same idea of turning off SSL2.

We need to add cipher/hash support for the following reasons: 1) compatibility: there are a critical mass of users that will break if we don't support the cipher/hash, 2) we need enough strong ciphers in which to pivot so we don't find our selves with zero strong ciphers. Right now we are sort of at risk if AES-GCM is attacked, since the block ciphers are presumed weak (known attacks -- though personally I see them as dubious value), and RC4 is known to be weak. SHA-224 doesn't help with number 2 because it's basically a weaker form of SHA-256. it's concievable that a SHA-224 could resist a SHA-256 attack, but it's not likely. To your point, though, it's also unlikely that SHA-224 is likely to show weaknesses that aren't also in SHA-256, so the risk isn't huge here either.

Given both of those SHA-224 is a push, when we implement this, it's best to implement this so that we can support any hash we have, then restrict the set (SHA-3 will need to be supported when it comes out, for instance), rather than building a one-off for each hash. This means we probably need to support this with buffering rather than creating one of each hash at startup (makes it easier to support more hashes on the server side as well).
Blocks: 1181807
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → 3.20
FWIW, we're probably deprecating SHA-244 from the spec in TLS 1.3. It's basically unused and I don't think it's worth bothering to implement for any TLS version.
Just to be clear
 - From what I know, implementing SHA-224 should be trivial when we have all other SHA-2 functions
 - no, it's most likely not necessary so it'd vote for disabling it by default.

(In reply to Dave Garrett from comment #11)
> (typo; I obviously meant SHA-224)
> 
> Some data:
> https://securitypitfalls.wordpress.com/2015/06/20/may-2015-scan-results/

That is for Server Key Exchange signature algorithms, not Certificate Request hashes (though it likely is a relatively good proxy for the latter).

Also, I'm not sure, but servers which require SHA-224 may not show up in the stats (though that number should be capped at around 4%).
(In reply to Robert Relyea from comment #9)
> I'm not at against adding SHA-224. It was a late addition because NIST
> decided they needed it, but I think it good to chime in on the principle
> that Ryan is trying to convey:
> 
> We have to balance the ciphers we support with the potential for future
> attacks. Adding SHA-224 means that if we run into an attack against SHA-224,
> we suddenly have opened up ourselves to that attack. The same idea of
> turning off SSL2.

That wasn't the point I was trying to convey, although it's related :)

My point was that many of the attacks have come from implementors trying to offer complete support for something, even though it's impractical to deploy or unusable in practice. Heartbleed is unquestionably the most visible of these, but even the recent OpenSSL vulnerability ( CVE-2015-1793 ) was well intentioned, but backfired.

My concern with SHA-224 support is that it becomes "yet another branch" everywhere, it implies (through implementation) that SHA-224 must be supported / can't be removed, even though it's quite unambiguously clear that SHA-224 cannot be used at large in interoperable systems (compare to the many implementations that don't support it).

Security of SHA-224 notwithstanding, I'm more concerned with the complexity (even if small) and the motivations (which don't appear to be made by actual need, since that can be demonstrated otherwise, but primarily based on 'well, we're here, so why not')

If there is actual need, rather than theoretical, it'd be good to articulate that somehow. Especially given the move *away* from it (and that Microsoft specifically and explicitly did not implement it), I'm not sure why we'd want to add it (and support it for the 10-ish years needed for RHEL, disabled or not)
> That wasn't the point I was trying to convey, although it's related :)

> Security of SHA-224 notwithstanding, I'm more concerned with the complexity (even if small) and the motivations
> (which don't appear to be made by actual need, since that can be demonstrated otherwise, but primarily based on
> 'well, we're here, so why not')

In that case Hubert's comments are pertinent. The risk SHA-224 support introduces in your scenario is completely negligible, since it's really a table entry. We already absorb the risk of the SHA-224 unique code because we already support it in the certificate signatures. The risk of adding it to the overall requirement is small because in order to support the other hashes, we'll need a table driven system anyway. SHA-224 is just another table entry.

In that case your argument is very much a push.

> Especially given the move *away* from it (and that Microsoft specifically and explicitly did not implement
> it), I'm not sure why we'd want to add it (and support it for the 10-ish years needed for RHEL, 
> disabled or not)

This is closer to my argument, which is SHA-224 is already going out of favor, do we really want a 10 year support cycle for it, which is probably the stronger argument.

bob
(In reply to Hubert Kario from comment #12)
> Also, I'm not sure, but servers which require SHA-224 may not show up in the
> stats (though that number should be capped at around 4%).

I now double checked, they will show up in those scans, which means there are no such ones in Alexa top 1 million that are also trusted by Firefox.

The point about this being stats of SKE rather than CR messages still stands.
This bug is to remove the current restriction that signature method function must be identical with PRF function.

We should support a list that's reasonable.

Whether or not to include SHA224 is just a small implementation detail.
Summary: NSS does not support all signature methods for Certificate Verify → Don't require that the signature method for certificate verify is identical with PRF, support alternatives
To update on what I mentioned in comment 10, SHA-224 is considered deprecated as of TLS 1.3 draft-08. That said, this is still an early draft. Things could change, but I haven't heard anyone care about SHA-224 being cut as of yet.

https://tools.ietf.org/html/draft-ietf-tls-tls13-08

I don't think it's really a problem for NSS to support SHA-224 if you really have to, but I don't see it being a particularly helpful move.
IMO, the discussion of SHA-224 is really a distraction from the real issue here, which is spec compliance, and agreement on signature algorithm between the client and server in TLS 1.2 . The server is allowed to request a different algorithm than the PRF in the CertificateRequest supported_signature_algorithms. The client should either honor that request, or fail in some way if it doesn't support that algorithm. We may want to add APIs to change the list of supported algorithms on either side.
Depends on: 923089
Assignee: nobody → kaie
I don't believe that this bug is worth fixing.  At least not without evidence that there are cases where it is necessary.

The cost of implementing this bug is non-trivial.  It means that the client needs to maintain a extra hashes of the session transcript (for any hashes that it prefers over the one used in the cipher suite).  It only gets to discard these once it receives the CertificateRequest from the server.

Similarly, the server has to support some number of extra hashes.  It has less certainty about what the client might prefer, so probably has to look at the signature_algorithms extension, though that only applies to the validation of signatures and so it might be completely misleading.

Currently an NSS server constrains its supported_signature_algorithms to those that use the PRF hash.  That is safe.  Since the client needs to maintain a hash that uses the PRF hash function, I can see no real harm in restricting the hash to the PRF hash.

The cost here isn't just code complexity, though that would certainly increase even though we already do multiple hashes for TLS 1.0 and 1.1.  It's the cost of holding all the extra state and performing the extra processing.  It's also conceivable that the state offload needed for stateless reject (i.e., HelloRetryRequest) in TLS 1.3 would not be possible.

Note: we removed SHA-224 in bug 1158489, let's not reverse that decision.
We have definitely seen servers in the real world where the signature algorithm requested for client auth is different from that of the PRF. These are usually some kind of odd hardware load balancers where no configuration is possibility. It has affected the ability of the NSS client to connect to such servers with TLS 1.2 . Downgrading to 1.1 works around the issue, but isn't a long-term fix. IMO, this bug is worth fixing. Buffering would be the right approach to do so, not maintaining a large number of hashes. I agree that it is a complicated fix. Especially since we need this to work not just in the regular code path, but PKCS#11 bypass code path as well. Right now, only SHA256 works in the bypass code path, not even SHA-1.
(In reply to Julien Pierre from comment #20)
> We have definitely seen servers in the real world where the signature
> algorithm requested for client auth is different from that of the PRF. These
> are usually some kind of odd hardware load balancers where no configuration
> is possibility. It has affected the ability of the NSS client to connect to
> such servers with TLS 1.2 . Downgrading to 1.1 works around the issue, but
> isn't a long-term fix. 

I'd be interested in more details here.  If this is rare, then taking the burden of implementing and maintaining the feature is still (in my opinion) not sufficient reason.

> IMO, this bug is worth fixing. Buffering would be the
> right approach to do so, not maintaining a large number of hashes. 

This won't be feasible for DTLS and might not be feasible for servers that operate at scale.

> I agree
> that it is a complicated fix. Especially since we need this to work not just
> in the regular code path, but PKCS#11 bypass code path as well. 

I believe that the right approach there is to remove the bypass code here and to fall back to PKCS#11.

> Right now, only SHA256 works in the bypass code path, not even SHA-1.

That's a good thing.  SHA-1 should not be used in security software.
Martin,
Unfortunately, our customers have been tight-lipped about what other devices they are running that require this. We just know they are out there. We don't use DTLS in Oracle products at this time, so this wouldn't be an issue for us. I can see how it would complicate the code path, though to separate both.
As far as bypass, we have given that as another workaround to the customer. Somehow, pesky customers want things to just work out of the box, though, without configuration changes. Bypass is on by default in several Oracle products. It still provides a very sizable performance advantage, and until such time as the PKCS#11 code path comes close, I don't think we will want to remove this code.
As far as the security of SHA-1, unfortunately the reality today is that SHA-1 is still used. I would agree that it shouldn't be enabled by default.

IMO, I think it makes sense to have APIs for applications to configure the allowed signature algorithms, just like we have for ciphers. The defaults should be secure (ie. only SHA-2 hashes), but applications or specific users that want less secure configurations for legacy interoperability reasons with some other program or infrastructure should be able to do so if they wish. Other libraries such as OpenSSL have this flexibility in terms of allowing the configuration of allowed signature algorithms. This might warrants filing separate RFEs.
(In reply to Martin Thomson [:mt:] from comment #19)
> I don't believe that this bug is worth fixing.  At least not without
> evidence that there are cases where it is necessary.

RFC 5246, section 7.4.4 (https://tools.ietf.org/html/rfc5246#section-7.4.4):

   -  Any certificates provided by the client MUST be signed using a
      hash/signature algorithm pair found in
      supported_signature_algorithms.

Even if the consensus of the TLS WG is that this is a wrong limitation, current Windows implementation of TLS 1.2 is strict in that regard. If the client certificate is signed with SHA-512 or SHA-384, connection will be aborted by the client upon receiving Certificate Request that lists only sha-256 hashes.

> The cost of implementing this bug is non-trivial.  It means that the client
> needs to maintain a extra hashes of the session transcript (for any hashes
> that it prefers over the one used in the cipher suite).  It only gets to
> discard these once it receives the CertificateRequest from the server.
> 
> Similarly, the server has to support some number of extra hashes.  It has
> less certainty about what the client might prefer, so probably has to look
> at the signature_algorithms extension, though that only applies to the
> validation of signatures and so it might be completely misleading.

Correct me if I'm wrong, but from commonly used TLS libraries, NSS is the only one with this limitation.
Neither GnuTLS nor OpenSSL have such limitation.

> Since the client needs to
> maintain a hash that uses the PRF hash function, I can see no real harm in
> restricting the hash to the PRF hash.

see above, it breaks interoperability

> Note: we removed SHA-224 in bug 1158489, let's not reverse that decision.

First: "You are not authorized to access bug 1158489."
Second: Like Julien said in Comment #20, sha-224 is secondary.

Finally, +1 for APIs to configure supported algorithms
(In reply to Hubert Kario from comment #23)
> (In reply to Martin Thomson [:mt:] from comment #19)
> > I don't believe that this bug is worth fixing.  At least not without
> > evidence that there are cases where it is necessary.
> 
> RFC 5246, section 7.4.4 (https://tools.ietf.org/html/rfc5246#section-7.4.4):
> 
>    -  Any certificates provided by the client MUST be signed using a
>       hash/signature algorithm pair found in
>       supported_signature_algorithms.
> 
> Even if the consensus of the TLS WG is that this is a wrong limitation,
> current Windows implementation of TLS 1.2 is strict in that regard. If the
> client certificate is signed with SHA-512 or SHA-384, connection will be
> aborted by the client upon receiving Certificate Request that lists only
> sha-256 hashes.

Let's distinguish between two separate questions here:

1. What certificates the client will supply.
2. What the client will sign with.

If the server sends SignatureAlgorithms of (SHA256, SHA512) and the client has
a SHA512-signed cert, all should be well because it can sign the CertificateVerify
with SHA256. If that doesn't work in NSS, then that's a bug and it's easily fixed.

What's not easily fixed is if the server demands SHA512 and the client has a
SHA512 signed cert, because then the client needs to maintain more hashes
(as Martin says). Are there any servers that behave this way? It seems like
they are asking for trouble



> > Since the client needs to
> > maintain a hash that uses the PRF hash function, I can see no real harm in
> > restricting the hash to the PRF hash.
> 
> see above, it breaks interoperability

It's not clear to me that it does, for the reasons above. Can you please explain
exactly what server behavior you think is a problem for NSS at present.
Just a reply to clarify the following comment for those not familiar with or involved in the TLS WG discussion on the topic.

(In reply to Hubert Kario from comment #23)
> RFC 5246, section 7.4.4 (https://tools.ietf.org/html/rfc5246#section-7.4.4):
> 
>    -  Any certificates provided by the client MUST be signed using a
>       hash/signature algorithm pair found in
>       supported_signature_algorithms.
> 
> Even if the consensus of the TLS WG is that this is a wrong limitation,
> current Windows implementation of TLS 1.2 is strict in that regard. If the
> client certificate is signed with SHA-512 or SHA-384, connection will be
> aborted by the client upon receiving Certificate Request that lists only
> sha-256 hashes.

Just to be a little more precise, we changed this text in the TLS 1.3 spec to be qualified with "if they are able to provide such a chain" and added that in the event of not being able to do so, a server "SHOULD continue the handshake by sending the client a certificate chain of its choice that may include algorithms that are not known to be supported by the client".

https://tools.ietf.org/html/draft-ietf-tls-tls13-12#section-6.3.4.1.1
https://tools.ietf.org/html/draft-ietf-tls-tls13-12#section-6.3.4.2

No promise is made with regard to how the client will respond to this, however. The decision to abort or proceed is up to the client. It is perfectly valid for a client to take this response and reject it completely if it can't verify and trust it. As a server still MUST follow the previous requirement if it is capable of doing so, this is only a fallback system, not a catchall to let servers reply with whatever they want and expect it to work. The point here is to shift the abort decision from the server to the client. A client designed to allow opportunistic encryption, for example, might decide to proceed regardless of authentication. Many others will just abort on the client side, but now at least having a bag of certs to examine for someone to attempt to analyze why it happened.

Any server that sends stuff to a client that did not specify support for it is risking an abort from the client, with both the old and new rules. This is all sort of tangential to the topic in this bug, however.
Eric, Dave,
The specific case that doesn't work today which concerns us most at Oracle is as follows :
- TLS 1.2 is negotiated between client and server (all other versions are OK)
- Server (non-NSS, opaque device) sends SHA1/RSA in supported_signature_algorithms and requests a client cert
- Client is NSS, using PKCS#11 bypass, and replies with a SHA-256 signature
- Server aborts the handshake because it's strictly enforcing the spec which says that the client must respond with a hash that matches one of the server's supported algorithms
- Customer complains that NSS is not compliant with TLS spec. IMO, reading the TLS 1.2 spec, which clearly states MUST, the behavior in NSS is not TLS 1.2 compliant
- the discussion with TLS 1.3 IMO doesn't apply here when it comes to 1.2 compliance, which is very clear. MUST leaves no room for interpretation. We should not reply with an unsupported signature algorithm. The current behavior is simply not spec compliant.
Postel's law says that you must be conservative in what you send, and liberal in what you accept. In this case, what NSS is doing is just plain wrong.
- customer does not like the workaround of disabling TLS 1.1 in the NSS client, which makes things work even with PKCS#11 bypass
- I believe disabling PKCS#11 bypass also makes this particular case work, but we haven't been able to verify this for ourselves as we don't have the actual server/device that the customer is using. I may be able to recreate such a server eventually using another TLS library. The test server can't be created with NSS itself since it's not currently capable of configuring the supported_signature_algorithms at the server side. That alone should be sufficient reason to add APIs to configure algorithms, IMO . Otherwise, these test cases can only be run in the context of interoperability tests. Is anyone still running the old NSS interoperability tests we developed at Sun ? They are no longer run at Oracle, and I don't think anyone setup the required machines outside of Sun.
>What's not easily fixed is if the server demands SHA512 and the client has a
>SHA512 signed cert, because then the client needs to maintain more hashes
>(as Martin says). Are there any servers that behave this way? It seems like
>they are asking for trouble

I haven't encountered this specific above case, but given that :
a) a server is allowed in the TLS 1.2 spec to demand this specific SHA512 algorithms
b) there are APIs in other TLS security libraries to configure signature algorithms at the server side
IMO, it's only a matter of time until :
c) an actual server implements a security policy requiring SHA512 for supported_signature_algorithms with TLS 1.2

Just because certain browsers have chosen not to support certain features in the protocol doesn't mean this won't happen. Not all TLS clients are browsers. And not every server is public internet facing.
(In reply to Julien Pierre from comment #26)
> - Server (non-NSS, opaque device) sends SHA1/RSA in
> supported_signature_algorithms and requests a client cert
> - Client is NSS, using PKCS#11 bypass, and replies with a SHA-256 signature

Yes, that is a bug, NSS should simply not send the certificate.  Is there an open bug on that?

BTW, I think that we maintain a SHA-1 hash, so this might be fixed in the way that you desire relatively easily.  I'm sure that if someone wrote a patch, any number of people here might review it.

> Postel's law says that you must be conservative in what you send, and
> liberal in what you accept. In this case, what NSS is doing is just plain
> wrong.

I've said this elsewhere, but Postel was wrong about this.

> - I believe disabling PKCS#11 bypass also makes this particular case work,

That's interesting; that scopes the bug to the bypass mode code.  That should make it easier to fix.
Martin,

We'll have to agree to disagree about Postel.

Re: the bug:

>Yes, that is a bug, NSS should simply not send the certificate.  Is there an open bug on that?

There was bug 915408 . It is marked fixed, but it only got fixed half-way, only in the PKCS#11 code path, not in the bypass case. IMO, the fix was incomplete. I would file a bug for the specific bypass case, but don't even have the test server to verify the behavior against at this point.

>That's interesting; that scopes the bug to the bypass mode code.  That should make it easier to fix.

I disagree that it should not send the certificate response. The client cert is SHA1/RSA. NSS should support creating a SHA1/RSA signature for the certificate response with TLS 1.2, like it does with previous versions of TLS. 

And indeed, Wan-Teh agreed with this assessment when fixing in bug 914408 . It is only the bypass case that was left unfixed.  Read the bug history.

Unfortunately, no one in Oracle was even aware this was the case, until it became a customer escalation. I understand the Mozilla community at large doesn't care about PKCS#11 bypass. We have no official development resources on NSS in Oracle - I have to work on many products that use multiple libraries as it stands, and it's unfortunate. I really wish I could contribute code to fix it myself, but I'm just one person, and there just aren't enough hours in the day, and my management just hasn't prioritized this. As originally designed and implemented, bypass was supposed to behave identically to NSS using softoken in terms of crypto, except bypassing the PKCS#11 APIs. It wasn't supposed to behave differently at the protocol level on the wire. But as of TLS 1.2, it differs on the wire, at least in this case, and that's unfortunate, IMO.

You can also read bug 948064 which is related, but server side. Our customers have not run into this particular case yet.
(In reply to Julien Pierre from comment #26)
> Eric, Dave,
> The specific case that doesn't work today which concerns us most at Oracle
> is as follows :
> - TLS 1.2 is negotiated between client and server (all other versions are OK)
> - Server (non-NSS, opaque device) sends SHA1/RSA in
> supported_signature_algorithms and requests a client cert
> - Client is NSS, using PKCS#11 bypass, and replies with a SHA-256 signature

If by "a SHA-256 signature" you mean one in the CertificateVerify, then this
sounds like a bug in NSS, then the NSS client should have instead sent
an alert because it can't generate an RSA-SHA1 signature.


> - Server aborts the handshake because it's strictly enforcing the spec which
> says that the client must respond with a hash that matches one of the
> server's supported algorithms

Yes, this sounds right. Though if the server 


> - the discussion with TLS 1.3 IMO doesn't apply here when it comes to 1.2
> compliance, which is very clear. MUST leaves no room for interpretation. We
> should not reply with an unsupported signature algorithm. The current
> behavior is simply not spec compliant.

Yes, and would be noncompliant in TLS 1.3.
(In reply to Eric Rescorla (:ekr) from comment #30)
> (In reply to Julien Pierre from comment #26)
> > Eric, Dave,
> > The specific case that doesn't work today which concerns us most at Oracle
> > is as follows :
> > - TLS 1.2 is negotiated between client and server (all other versions are OK)
> > - Server (non-NSS, opaque device) sends SHA1/RSA in
> > supported_signature_algorithms and requests a client cert
> > - Client is NSS, using PKCS#11 bypass, and replies with a SHA-256 signature
> 
> If by "a SHA-256 signature" you mean one in the CertificateVerify, then this
> sounds like a bug in NSS, then the NSS client should have instead sent
> an alert because it can't generate an RSA-SHA1 signature.
> 
> 
> > - Server aborts the handshake because it's strictly enforcing the spec which
> > says that the client must respond with a hash that matches one of the
> > server's supported algorithms
> 
> Yes, this sounds right. Though if the server 

If the server would accept SHA-256 (and we know that it is able to do
the hash or it couldn't do 1.3) then should advertise it.
> 
> 
> > - the discussion with TLS 1.3 IMO doesn't apply here when it comes to 1.2
> > compliance, which is very clear. MUST leaves no room for interpretation. We
> > should not reply with an unsupported signature algorithm. The current
> > behavior is simply not spec compliant.
> 
> Yes, and would be noncompliant in TLS 1.3.
(In reply to Eric Rescorla (:ekr) from comment #30)
> 
> If by "a SHA-256 signature" you mean one in the CertificateVerify, 

Yes, that's what I meant.

> then this
> sounds like a bug in NSS, then the NSS client should have instead sent
> an alert because it can't generate an RSA-SHA1 signature.

Yes, that would be one RFC5246-compliant response, but certainly not ideal for interoperability purposes.
IMO, the NSS client could generate an RSA-SHA1 signature, like it already can when using non-bypass mode, or when using TLS versions < 1.2 .

> > - the discussion with TLS 1.3 IMO doesn't apply here when it comes to 1.2
> > compliance, which is very clear. MUST leaves no room for interpretation. We
> > should not reply with an unsupported signature algorithm. The current
> > behavior is simply not spec compliant.
> 
> Yes, and would be noncompliant in TLS 1.3.

Right. But unless we plan on dropping TLS 1.2 from NSS, we need to support both 1.2 and the upcoming 1.3, even though they have different semantics & restrictions.
Maybe the buffering and/or multiple hashes only need to be done for 1.2, and not 1.3.
(In reply to Julien Pierre from comment #32)
> (In reply to Eric Rescorla (:ekr) from comment #30)
> > 
> > If by "a SHA-256 signature" you mean one in the CertificateVerify, 
> 
> Yes, that's what I meant.
> 
> > then this
> > sounds like a bug in NSS, then the NSS client should have instead sent
> > an alert because it can't generate an RSA-SHA1 signature.
> 
> Yes, that would be one RFC5246-compliant response, but certainly not ideal
> for interoperability purposes.
> IMO, the NSS client could generate an RSA-SHA1 signature, like it already
> can when using non-bypass mode, or when using TLS versions < 1.2 .

Yes, it could do that and I agree it would be better. As I said, we would be happy to review a patch that fixed this for bypass mode. However, writing a patch like this is probably not going to be a high priority for Mozilla.


> > > - the discussion with TLS 1.3 IMO doesn't apply here when it comes to 1.2
> > > compliance, which is very clear. MUST leaves no room for interpretation. We
> > > should not reply with an unsupported signature algorithm. The current
> > > behavior is simply not spec compliant.
> > 
> > Yes, and would be noncompliant in TLS 1.3.
> 
> Right. But unless we plan on dropping TLS 1.2 from NSS, we need to support
> both 1.2 and the upcoming 1.3, even though they have different semantics &
> restrictions.
> Maybe the buffering and/or multiple hashes only need to be done for 1.2, and
> not 1.3.

That was just intended as a clarifying point. Note also that the TLS 1.3 code does not implement bypass at all.
(In reply to Eric Rescorla (:ekr) from comment #31)
> If the server would accept SHA-256 (and we know that it is able to do
> the hash or it couldn't do 1.3) then should advertise it.

In the case I was discussing, the TLS server is 1.2 only - not 1.3, and isn't advertising SHA-256 in CertificateRequest because it's not willing to accept it for CertificateVerify. 

Even if a server is 1.3-capable, if the client advertises 1.2 in ClientHello, then 1.2 is what gets negotiated, and thus the 1.2 semantics should apply, meaning the client should send a CertificateVerify with one of the server's supported_signature_algorithms, or an alert. A TLS 1.2 client would have no way of knowing that a 1.3 server can accept an unspecified signature algorithm. Only a TLS 1.3 client would know this, and at that point, 1.3 would be negotiated, not 1.2.

(In reply to Eric Rescorla (:ekr) from comment #33)
> Yes, it could do that and I agree it would be better. As I said, we would be
> happy to review a patch that fixed this for bypass mode. However, writing a
> patch like this is probably not going to be a high priority for Mozilla.

If I ever manage to find the time, I will submit a patch. Right now I am still stuck on reproducing the test case - creating a server that requests SHA-1 in supported_signature_algorithms. This requires using another TLS library, as mentioned. Once I get past that, it may be easier for me to create the patch and submit it.

> That was just intended as a clarifying point. Note also that the TLS 1.3
> code does not implement bypass at all.

Yes, I understand that. This is probably not be something I'll be able to fix.
(In reply to Julien Pierre from comment #34)
> (In reply to Eric Rescorla (:ekr) from comment #31)
> > If the server would accept SHA-256 (and we know that it is able to do
> > the hash or it couldn't do 1.3) then should advertise it.
> 
> In the case I was discussing, the TLS server is 1.2 only - not 1.3, and
> isn't advertising SHA-256 in CertificateRequest because it's not willing to
> accept it for CertificateVerify. 

This was a typo on my part. TLS 1.2 requires having some SHA-256 support
(because you need it for the PRF).


> (In reply to Eric Rescorla (:ekr) from comment #33)
> > Yes, it could do that and I agree it would be better. As I said, we would be
> > happy to review a patch that fixed this for bypass mode. However, writing a
> > patch like this is probably not going to be a high priority for Mozilla.
> 
> If I ever manage to find the time, I will submit a patch. Right now I am
> still stuck on reproducing the test case - creating a server that requests
> SHA-1 in supported_signature_algorithms. This requires using another TLS
> library, as mentioned. Once I get past that, it may be easier for me to
> create the patch and submit it.

You should be able to reproduce this case with NSS by using the SSL_SignaturePrefSet()
API.
(In reply to Eric Rescorla (:ekr) from comment #35)
> This was a typo on my part. TLS 1.2 requires having some SHA-256 support
> (because you need it for the PRF).

OK, that makes more sense.

> > If I ever manage to find the time, I will submit a patch. Right now I am
> > still stuck on reproducing the test case - creating a server that requests
> > SHA-1 in supported_signature_algorithms. This requires using another TLS
> > library, as mentioned. Once I get past that, it may be easier for me to
> > create the patch and submit it.
> 
> You should be able to reproduce this case with NSS by using the
> SSL_SignaturePrefSet()
> API.

Great, thanks for that ! I didn't know we had such an API. Does it work for both sides ? If so, maybe I can solve the client problem too by calling this API at the client side. Still need to make PKCS#11 bypass honor it too, though.
I believe it works for both sides:
/*
** Control for TLS signature algorithms for TLS 1.2 only.
**
** This governs what signature algorithms are sent by a client in the
** signature_algorithms extension.  A client will not accept a signature from a
** server unless it uses an enabled algorithm.
**
** This also governs what the server sends in the supported_signature_algorithms
** field of a CertificateRequest.  It also changes what the server uses to sign
** ServerKeyExchange: a server uses the first entry from this list that is
** compatible with the client's advertised signature_algorithms extension and
** the selected server certificate.
**
** Omitting SHA-256 from this list might be foolish.  Support is mandatory in
** TLS 1.2 and there might be interoperability issues.  For a server, NSS only
** supports SHA-256 for verifying a TLS 1.2 CertificateVerify.  This list needs
** to include SHA-256 if client authentication is requested or required, or
** creating a CertificateRequest will fail.
*/
(In reply to Eric Rescorla (:ekr) from comment #24)
> (In reply to Hubert Kario from comment #23)
> > (In reply to Martin Thomson [:mt:] from comment #19)
> > > I don't believe that this bug is worth fixing.  At least not without
> > > evidence that there are cases where it is necessary.
> > 
> > RFC 5246, section 7.4.4 (https://tools.ietf.org/html/rfc5246#section-7.4.4):
> > 
> >    -  Any certificates provided by the client MUST be signed using a
> >       hash/signature algorithm pair found in
> >       supported_signature_algorithms.
> > 
> > Even if the consensus of the TLS WG is that this is a wrong limitation,
> > current Windows implementation of TLS 1.2 is strict in that regard. If the
> > client certificate is signed with SHA-512 or SHA-384, connection will be
> > aborted by the client upon receiving Certificate Request that lists only
> > sha-256 hashes.
> 
> Let's distinguish between two separate questions here:
> 
> 1. What certificates the client will supply.
> 2. What the client will sign with.
> 
> If the server sends SignatureAlgorithms of (SHA256, SHA512) and the client
> has
> a SHA512-signed cert, all should be well because it can sign the
> CertificateVerify
> with SHA256. If that doesn't work in NSS, then that's a bug and it's easily
> fixed.

it does send CertificateVerify with SHA-256, the problem is with servers which don't advertise support for SHA-256 nor SHA-1

> What's not easily fixed is if the server demands SHA512 and the client has a
> SHA512 signed cert, because then the client needs to maintain more hashes
> (as Martin says). Are there any servers that behave this way? It seems like
> they are asking for trouble

this part of problem is more relevant the NSS-as-server: the server then always asks for SHA-256, windows as a client with client certificate signed with SHA-512 won't connect to NSS.

> > > Since the client needs to
> > > maintain a hash that uses the PRF hash function, I can see no real harm in
> > > restricting the hash to the PRF hash.
> > 
> > see above, it breaks interoperability
> 
> It's not clear to me that it does, for the reasons above. Can you please
> explain
> exactly what server behavior you think is a problem for NSS at present.

NSS-as-server asks only for sha-256, not all clients support sha-256

for NSS-as-client the real problems will start when NSS gains support for ciphers with SHA-384 PRF, as there are servers which accept only SHA-256 independent of PRF (and not because the certificate is sha-256)
Alias: flexible-certverify
Target Milestone: 3.20 → 3.25
(In reply to Martin Thomson [:mt:] from comment #19)
> I don't believe that this bug is worth fixing.  At least not without
> evidence that there are cases where it is necessary.

One example is in bug 923089 comment 14.

GnuTLS cannot interoperate with NSS when using client auth and TLS_DHE_DSS_WITH_AES_256_GCM_SHA384
That is a good reason.  Thanks Kai.
Attached patch work-in-progress-v1 (obsolete) — — Splinter Review 
Initial patch, which works in today's default configuration.

- it passes SSL tests
- it passes ssl_gtest, except test RequestClientAuthWithoutSha256,
  because that test expects a failing scenario that no longer fails
- it passes Hubert's interop test with GnuTLS

However
- TLS 1.3 not tested, and probably doesn't work yet, because I haven't
  touched it to make use of the buffered handshakes for cert verify.

More things to do:
- I'm not sure if we really should rename sha/md5 to nsHash1/nsHash2
- bypass mode not yet covered.
  Given that until today, bypass hasn't even supported the backupHash,
  should I bother to work on that?
- Optimization. We probably should skip buffering the handshake messages
  as soon as it's no longer necessary.
Kai,
I would very much appreciate if you could fix the bypass case too, at least for TLS 1.2, but obviously can't tell you what to do. I wish I could help, but I'm still swamped in non-NSS tasks at Oracle.
Attached patch patch v2 (obsolete) — — Splinter Review 
This patch might be ready for review, except:
- doesn't work in TLS 1.3 mode. It builds, but triggers assertions.
  It would be nice to have that fixed by EKR, or with help from EKR,
  because some assumptions made by the TLS 1.3 code are no longer correct.
- no support for bypass mode yet
  (it still behaves as before)

What works:
- it passes SSL tests
- it passes Hubert's interop test with GnuTLS
- it passes ssl_gtest, except test RequestClientAuthWithoutSha256,
  because that test expects a failing scenario that no longer fails

We need to decide what to do about RequestClientAuthWithoutSha256.
Should we remove it?

Regarding renaming:
I've reverted the renaming of the md5/sha attributes.
Fewer changes, easier to review, less conflicts.

I've implemented the optimization that I mentioned in the previous comment
(stop buffering hs messages when we don't need any more for cert verify.)
That simplified the code.
Attachment #8755661 - Attachment is obsolete: true
Attachment #8755849 - Flags: review?(martin.thomson)
Attachment #8755849 - Flags: feedback?(ekr)
Also, I've added a NSS_FORCE_CERT_VERIFY_HASH environment variable, only enabled in debug builds, which can be used to enforce the use of SHA256 as the hash for certificate verify on the server side.

When using a SHA384-ciphersuite like :009D, it can be used to test that the different hash for certificate indeed work.

I wonder if we need more advanced testing. Do we need testing that works in optimized builds, too? Should it be automated? If yes, should we add a command line parameter to selfserv/tstclnt that enforces the use of a specific hash for certificate verify?
(In reply to Kai Engert (:kaie) from comment #44)
> I wonder if we need more advanced testing. Do we need testing that works in
> optimized builds, too? Should it be automated? If yes, should we add a
> command line parameter to selfserv/tstclnt that enforces the use of a
> specific hash for certificate verify?

The alternative is to rely on Hubert's test suite, which currently isn't published, but which RH could run on a separate builder, administrated by RH, reporting results to our NSS buildbot.
Kai, what assumptions are those? TLS 1.3 actually requires that the signatures be done with the handshake hash.
(In reply to Eric Rescorla (:ekr) from comment #46)
> Kai, what assumptions are those? 

I ran into this assertion in ssl3_SendCertificateVerify:
  PORT_Assert(!isTLS13);

You also have a comment there:

         * In TLS 1.3, we always sign H(Context, Hash(handshake))
         * where:
         *
         * H is the negotiated signature hash and
         * Hash is the cipher-suite specific handshake hash
         * Generally this means that Hash is SHA-256.
         *
         * We need code to negotiate H.

When removing the above assertion, I ran into another one.

But...


> TLS 1.3 actually requires that the
> signatures be done with the handshake hash.

This is very helpful to know, thanks. I'll try to adjust the new code to check for TLS 1.3 and behave accordingly.
(In reply to Kai Engert (:kaie) from comment #45)
> (In reply to Kai Engert (:kaie) from comment #44)
> > I wonder if we need more advanced testing. Do we need testing that works in
> > optimized builds, too? Should it be automated? If yes, should we add a
> > command line parameter to selfserv/tstclnt that enforces the use of a
> > specific hash for certificate verify?
> 
> The alternative is to rely on Hubert's test suite, which currently isn't
> published, but which RH could run on a separate builder, administrated by
> RH, reporting results to our NSS buildbot.

I'm not sure which exact tests you're referring to. Checks if server will accept any of the signatures on Certificate Verify we agreed as necessary (sha1, sha256, sha384 and sha512) are public:
https://github.com/tomato42/tlsfuzzer/blob/master/scripts/test-rsa-sigs-on-certificate-verify.py
negative check to make sure that MD5 are not enabled by mistake are also public:
https://github.com/tomato42/tlsfuzzer/blob/master/scripts/test-certificate-verify.py
Kai, to be strictly accurate, say you are signing with ECDSA/P-256/SHA-256, but you negotiated a cipher suite with SHA-384 as the PRF, you sign:

SHA-256(Context, SHA-384(handshake))
(In reply to Eric Rescorla (:ekr) from comment #49)
> Kai, to be strictly accurate, say you are signing with ECDSA/P-256/SHA-256,
> but you negotiated a cipher suite with SHA-384 as the PRF, you sign:
> 
> SHA-256(Context, SHA-384(handshake))

Thanks. I assume you already handle this well in the TLS 1.3 code. For me, I think the important detail is, when using TLS 1.3, that we don't need to go through the flexible-certverify code implemented in this bug, and we should be ok.
Attached patch patch v3 (obsolete) — — Splinter Review 
This slightly modified version of the patch already works in TLS 1.3, and passes ssl_gtest.

I've modified the test to expect success, when restricting the hash.

It also passed in all other build combinations I have tested, in both bypass-allowed and bypass-not-allowed modes. I was initially confused why the test always worked. Then I got the impression you apparently don't have a client cert set up on the client side, which is why we never get to actually using a client cert? (The test is to request, not require.)
Attachment #8755849 - Attachment is obsolete: true
Attachment #8755849 - Flags: review?(martin.thomson)
Attachment #8755849 - Flags: feedback?(ekr)
Hubert, yes, you've pointed to those tests, but I haven't been able to learn them yet, and I'm reluctant to use them because of the additional dependencies. Yes, you have provided some tests, but we don't have anything, with little external dependencies, that could be easily included into the NSS project.

The alternatives I see are:
- enhance the NSS tools, so we could test the code that works with restricted hashes,
  without having to depend on anything externally
or
- setup a separate test machine, that uses the external tests which we have,
  which is configured to use all those external dependencies
Comment on attachment 8755877 [details] [diff] [review]
patch v3

Review of attachment 8755877 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure that I like this design much.

I see a few options for this that might be better:

1. Store hashes always.  This would remove all the code around whether we have a hash and it would just use the buffer.  The downside of this is that in TLS 1.3, especially for DTLS, we are likely to want to not do this.

2. Look at the version of the protocol and do one of three things:

  a. TLS 1.0 and 1.1 (and SSL3?) maintain SHA1 and MD5 hashes for messages.
  b. TLS 1.2 maintain a buffer of all messages.
  c. TLS 1.3 maintain a hash dependent on the PRF.

Can you put this up on rietveld next time please?  It's hard reviewing this without good context.

::: external_tests/ssl_gtest/ssl_loopback_unittest.cc
@@ +474,1 @@
>  TEST_P(TlsConnectTls12, RequestClientAuthWithoutSha256) {

I would change this to RequestClientAuthWithSha384.  The comment might then be omitted because it isn't that special.

::: lib/ssl/ssl3con.c
@@ +4480,5 @@
>      }
>  
>      if (ss->ssl3.hs.messages.len > 0) {
>          if (ssl3_UpdateHandshakeHashes(ss, ss->ssl3.hs.messages.buf,
> +                                       ss->ssl3.hs.messages.len, PR_FALSE) != 

trailing ws

@@ +4968,5 @@
> +                return SECFailure;
> +            }
> +        }
> +    }
> +#endif

This is fine for local debugging, but I don't really see much value in doing this.

@@ +6752,2 @@
>      if (ss->ssl3.hs.hashType == handshake_hash_single &&
> +        ss->ssl3.hs.certVerifyHash != ssl_hash_none) {

I don't see the point of certVerifyHash.  Surely if you have to defer the question of what to use (which only happens in TLS 1.2), then you can work out what hash to use right here.

@@ +7659,3 @@
>  static void
> +ssl3_DecideCertVerifyHash(sslSocket *ss,
> +                          const SECItem *algorithms)

Why does this function not copy from code that we use on the server side: ssl3_PickSignatureHashAlgorithm().  It should be the same logic.  I can see why you would want to only store the decision and not the entire set of peer preferences, but keeping the logic symmetric makes a lot of sense.  I would rename ss->ssl3.hs.clientSigAndHash to ss->ssl3.hs.peerSigAndHash.

@@ +7670,5 @@
> +
> +    if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
> +        /* TLS 1.3 always uses the handshake hash, nothing to decide. */
> +        return;
> +    }

This function shouldn't even be called by TLS 1.3 code.

@@ +10123,5 @@
> +        } else {
> +            forceHashAlg = ssl3_GetSuitePrfHash(ss);
> +        }
> +    }
> +#endif

As before, I don't think that we should have debug-only code like this.  Can you replicate this behaviour in other ways?  There are many terrible things that you can do in the gtests by poking at socket internals.

::: lib/ssl/sslimpl.h
@@ +869,5 @@
>  
>      /* This group of members is used for handshake running hashes. */
>      SSL3HandshakeHashType hashType;
>      sslBuffer messages; /* Accumulated handshake messages */
> +    PRBool hsBufferingDone;

Why not just use hashType to signal what you want to store?
(In reply to Martin Thomson [:mt:] from comment #53)
> @@ +6752,2 @@
> >      if (ss->ssl3.hs.hashType == handshake_hash_single &&
> > +        ss->ssl3.hs.certVerifyHash != ssl_hash_none) {
> 
> I don't see the point of certVerifyHash.  Surely if you have to defer the
> question of what to use (which only happens in TLS 1.2), then you can work
> out what hash to use right here.

In order to decide, which hash to use for certificate_verify, we require the list of signature algorithms sent by the server.

The current patch processes the list at the time the client receives it, when handling the certificate_request, makes the decision and stores it in certVerifyHash. Therefore it isn't necessary to store the list of supported server algorithms, it is released immediately.

If we decided to defer the decision until we need the information, instead of adding certVerifyHash, we'd have to add a new attribute serverSigAndHash to SSL3HandshakeState (similar to the existing clientSigAndHash used by ssl3_PickSignatureHashAlgorithm). Or, the existing attribute clientSigAndHash would have to be renamed to peerSigAndHash, and used by both client and server code.
It's not as simple as "just keep a buffer, and calculate when necessary".

This is particularly true on the server side, in ssl3_HandleCertificateVerify().

At the time the existing code calculates the hash for verification, we have already received additional handshake messages, which must be excluded from the calculation.

This means, we must either change the code to calculate the hashes at a different (earlier) time, and carry them forward to the right place, or, to stop buffering when we know that we've received all the relevant messages. The patch for review implemented the second option, in order to minimize the data that we must pass along with function parameters.

My thinking was, because the buffering is only required for the potential secondary certVerify hash, it might be fine to have that buffer in addition to the regular hash - because the regular hash might be required at multiple times (finished message, extended master secret message).

FYI.
Attached patch Patch v9 (obsolete) — — Splinter Review 
Patch v9 with 20 lines context.

Also uploaded here: https://codereview.appspot.com/300860043
Attachment #8755877 - Attachment is obsolete: true
Attachment #8758051 - Flags: review?(martin.thomson)
Martin, I have addressed most of your requests.

This patch appears to work in all modes I have tested, with and without bypass, with TLS 1.3 and gtests, and passes our interoperability tests.

I've removed the code that handles the debugging environment variable.


(In reply to Martin Thomson [:mt:] from comment #53)
> 2. Look at the version of the protocol and do one of three things:
> 
>   a. TLS 1.0 and 1.1 (and SSL3?) maintain SHA1 and MD5 hashes for messages.
>   b. TLS 1.2 maintain a buffer of all messages.
>   c. TLS 1.3 maintain a hash dependent on the PRF.

I've chosen this approach.

Because of this change, with TLS 1.2, I must pass along msgLenForCertVerify, as explained in 55.


> @@ +6752,2 @@
> >      if (ss->ssl3.hs.hashType == handshake_hash_single &&
> > +        ss->ssl3.hs.certVerifyHash != ssl_hash_none) {
> 
> I don't see the point of certVerifyHash.  Surely if you have to defer the
> question of what to use (which only happens in TLS 1.2), then you can work
> out what hash to use right here.

Please see my explanation in comment 54.

It seems we must store one thing or the other. Storing the decided hash seems to be more trivial than storing the list of supported server hashes we received?


> 
> @@ +7659,3 @@
> >  static void
> > +ssl3_DecideCertVerifyHash(sslSocket *ss,
> > +                          const SECItem *algorithms)
> 
> Why does this function not copy from code that we use on the server side:
> ssl3_PickSignatureHashAlgorithm().  It should be the same logic.  I can see
> why you would want to only store the decision and not the entire set of peer
> preferences, but keeping the logic symmetric makes a lot of sense.  I would
> rename ss->ssl3.hs.clientSigAndHash to ss->ssl3.hs.peerSigAndHash.

I had not seen this last sentence until now. :-(

I should have read your comment more carefully.

So, you are in favor of copying the whole array and keeping it around, only for using it once?

If that's what you request, I can change it, but I'm not convince it's the best thing to do.

The ssl3_DecideTls12CertVerifyHash function, as copied from the old destroy-if-needed function, has additional client specific logic, like calling ssl3_ExtractClientKeyInfo and deriving a SHA-1 preference. It has version checks, which are redundant for the ssl3_DecideTls12CertVerifyHash call, which is only called for TLS 1.2. Function ssl3_PickSignatureHashAlgorithm does a double iteration, iterating over both server and client preferences, but in ssl3_DecideTls12CertVerifyHash, we only compare the server hashes with the cipher suite hash.
Having two separate functions appears to be simpler than having a common one.
However, I have taken the additional policy check, and added it to ssl3_DecideTls12CertVerifyHash.

Is this convincing, or do you still prefer the common function?
Comment on attachment 8758051 [details] [diff] [review]
Patch v9

Review of attachment 8758051 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for throwing this on rietveld.  I started on splinter and copied my other comments over.

I think that this is making good progress.  Most of my suggestions should simplify this.

::: lib/ssl/ssl3con.c
@@ +3932,5 @@
>  
> +static CK_MECHANISM_TYPE
> +ssl3_GetTls12PrfHashMechanism(sslSocket *ss)
> +{
> +    return ssl3_GetTls12HashMechanismByHashType(ss->ssl3.hs.suite_def->prf_hash);

A request for these functions, can you remove the "Tls12" from the name.  A comment noting that this is only valid for TLS 1.2 *and greater* should suffice.

@@ +4419,5 @@
>              ss->ssl3.hs.sha_clone = ssl3_GetTls12BypassHashCloneFunc(ss);
>              ss->ssl3.hs.hashType = handshake_hash_single;
>              ss->ssl3.hs.sha_obj->begin(ss->ssl3.hs.sha_cx);
> +        } else if (ss->version == SSL_LIBRARY_VERSION_TLS_1_2) {
> +            ss->ssl3.hs.hashType = handshake_hash_single;

I think that we need to use a different enumeration value for this.  In 1.3, you are retaining all the code that constructs the hash and does a bunch of things.  In 1.2, you just maintain the handshake messages.

@@ +4562,5 @@
> +    } else if (ss->version == SSL_LIBRARY_VERSION_TLS_1_2) {
> +        append = PR_TRUE;
> +    } else if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
> +        calculate = PR_TRUE;
> +    }

Rather than rely on this complicated logic, rely on a new value to the hashType:

handshake_hash_unknown = buffer and quit (not sure yet)
handshake_hash_record = buffer and quit (1.2)
handshake_hash_single = update one hash (1.3)
handshake_hash_combo = update both sha1 and md5 (< 1.2)

@@ +6780,5 @@
>  }
>  
> +static SECStatus
> +ssl3_ComputeHash(sslSocket *ss, unsigned char *buf, unsigned int len,
> +                 SSLHashType hashAlg, SSL3Hashes *hashes)

This function seems to duplicate some of the code in ssl3_ComputeHandshakeHashes().  Is there any way that it can be used there?

@@ +6792,5 @@
> +        const SECOidData *hashOid =
> +            SECOID_FindOIDByMechanism(ssl3_GetTls12HashMechanismByHashType(hashAlg));
> +
> +        if (hashOid) {
> +            h_obj = HASH_GetRawHashObject(HASH_GetHashTypeByOidTag(hashOid->offset));

Do you need to free hashOid or h_obj?

@@ +6796,5 @@
> +            h_obj = HASH_GetRawHashObject(HASH_GetHashTypeByOidTag(hashOid->offset));
> +        }
> +        if (!h_obj) {
> +            ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
> +            rv = SECFailure;

This can just be an early return.

@@ +6865,5 @@
>           * H is the negotiated signature hash and
>           * Hash is the cipher-suite specific handshake hash
>           * Generally this means that Hash is SHA-256.
>           *
> +         * We need code to negotiate H.

Maybe you can instead say that H is negotiated based on the CertificateRequest and that negotiation is done in ssl3_HandleCertificateRequest().

@@ +10080,5 @@
> +            }
> +            if (hashAlg == ssl_hash_sha1) {
> +                supportsSha1 = PR_TRUE;
> +            } else if (hashAlg == ss->ssl3.hs.suite_def->prf_hash) {
> +                supportsHandshakeHash = PR_TRUE;

See below.

@@ +10093,5 @@
> +    } else if (supportsHandshakeHash) {
> +        ss->ssl3.hs.tls12CertVerifyHash = ssl_hash_none; /* Use suite PRF hash. */
> +    } else {
> +        ss->ssl3.hs.tls12CertVerifyHash = otherHashAlg;
> +    }

Even if we keep this, I think that this is more complicated that necessary.  Since we maintain the handshake messages, we can do any hash we like.  The handshake hash isn't really important in that, so you can remove checks around it.  That reduces this to:

if (supportsSha1 && preferSha1) ss->ssl3.hs.tls12CertVerifyHash = ssl_hash_sha1; 
else ss->ssl3.hs.tls12CertVerifyHash = preferredHash;

@@ +10391,5 @@
>          errCode = SEC_ERROR_LIBRARY_FAILURE;
>          goto alert_loser;
>      }
>  
>      if (isTLS12) {

Use the new enumeration type here instead.

@@ +12398,5 @@
>              sender = ss->sec.isServer ? sender_client : sender_server;
>              rSpec = ss->ssl3.crSpec;
>          }
> +        if (type == certificate_verify && ss->version == SSL_LIBRARY_VERSION_TLS_1_2) {
> +            msgLenForCertVerify = ss->ssl3.hs.messages.len;

I was going to suggest that you move this to ssl3_ComputeHandshakeHashes() but I see that you are skipping the hashing for this message.  Why not tweak the value of computeHashes instead?

Where does ss->ssl3.hs.messages.len point to in the transcript?  (If you keep this code, you will need to include a comment explaining what you are doing; it's not obvious.)

@@ +13844,5 @@
>          if (ss->ssl3.hs.hashType == handshake_hash_combo) {
>              SHA1_DestroyContext((SHA1Context *)ss->ssl3.hs.sha_cx, PR_FALSE);
>              MD5_DestroyContext((MD5Context *)ss->ssl3.hs.md5_cx, PR_FALSE);
>          } else if (ss->ssl3.hs.hashType == handshake_hash_single) {
> +            if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {

See previous comment regarding the enum.
(In reply to Kai Engert (:kaie) from comment #57)
> Because of this change, with TLS 1.2, I must pass along msgLenForCertVerify,
> as explained in 55.

I'm not sure that you need this.  There's already a provision for messages that shouldn't be included in the hash, can you use that?  See my review for more.
 
> > I don't see the point of certVerifyHash.  Surely if you have to defer the
> > question of what to use (which only happens in TLS 1.2), then you can work
> > out what hash to use right here.
> 
> Please see my explanation in comment 54.
> 
> It seems we must store one thing or the other. Storing the decided hash
> seems to be more trivial than storing the list of supported server hashes we
> received?

Yes, I see why you are using it now.  It's probably best this way.

> So, you are in favor of copying the whole array and keeping it around, only
> for using it once?

I think that you have the right of it.

> The ssl3_DecideTls12CertVerifyHash function, as copied from the old
> destroy-if-needed function, has additional client specific logic, like
> calling ssl3_ExtractClientKeyInfo and deriving a SHA-1 preference. It has
> version checks, which are redundant for the ssl3_DecideTls12CertVerifyHash
> call, which is only called for TLS 1.2. Function
> ssl3_PickSignatureHashAlgorithm does a double iteration, iterating over both
> server and client preferences, but in ssl3_DecideTls12CertVerifyHash, we
> only compare the server hashes with the cipher suite hash.
> Having two separate functions appears to be simpler than having a common one.
> However, I have taken the additional policy check, and added it to
> ssl3_DecideTls12CertVerifyHash.
> 
> Is this convincing, or do you still prefer the common function?

I still think that we should be using our preferences rather than that of our peer.  I know that means having double iteration, but that's a trivial cost to bear.  Looking at ssl3_PickSignatureHashAlgorithm(), it looks easy to fix:

* pass the extension as arguments (const char *peerAlgorithms, unsigned int length)
* set the signature algorithm based on the cipher suite if you are a server, or the key if you are a client
* keep the loop

The more I think about the SHA-1 preference, the more I think that we should be ignoring it.  SHA-1 is pretty badly busted and we know that all TLS 1.2 implementations have a better hash function.  That removes that concern.
(In reply to Martin Thomson [:mt:] from comment #58)
> @@ +6792,5 @@
> > +        const SECOidData *hashOid =
> > +            SECOID_FindOIDByMechanism(ssl3_GetTls12HashMechanismByHashType(hashAlg));
> > +
> > +        if (hashOid) {
> > +            h_obj = HASH_GetRawHashObject(HASH_GetHashTypeByOidTag(hashOid->offset));
> 
> Do you need to free hashOid or h_obj?

I checked existing uses of SECOID_FindOIDByMechanism and HASH_GetRawHashObject, but none does any cleanup on the returned objects.
(In reply to Martin Thomson [:mt:] from comment #58)
> @@ +10093,5 @@
> > +    } else if (supportsHandshakeHash) {
> > +        ss->ssl3.hs.tls12CertVerifyHash = ssl_hash_none; /* Use suite PRF hash. */
> > +    } else {
> > +        ss->ssl3.hs.tls12CertVerifyHash = otherHashAlg;
> > +    }
> 
> Even if we keep this, I think that this is more complicated that necessary. 
> Since we maintain the handshake messages, we can do any hash we like.  The
> handshake hash isn't really important in that, so you can remove checks
> around it.  That reduces this to:
> 
> if (supportsSha1 && preferSha1) ss->ssl3.hs.tls12CertVerifyHash =
> ssl_hash_sha1; 
> else ss->ssl3.hs.tls12CertVerifyHash = preferredHash;

But, what is preferredHash?
We don't have that variable currently.
We'd need new logic to decide about preferredHash.

If we prefer the suite hash, then it doesn't get simpler, because the existing check implements that preference decision.
For the record, ths TLS 1.3 code hasn't implemented bypass yet, it crashes when attempting to use it.

In tls13_ComputeHandshakeHashes it assumes that is non-null, but it's null in bypass mode.

Therefore I'd like to declare that handling TLS 1.3 bypass mode is outside the scope of this bug.
TLS 1.3 is not compatible with bypass and isn't intended to be. I thought that I had put in guards that caught that, but maybe they are just assert...
(In reply to Eric Rescorla (:ekr) from comment #63)
> TLS 1.3 is not compatible with bypass and isn't intended to be. I thought
> that I had put in guards that caught that, but maybe they are just assert...

I've filed that crash as bug 1276898.
(In reply to Martin Thomson [:mt:] from comment #58)
> @@ +12398,5 @@
> >              sender = ss->sec.isServer ? sender_client : sender_server;
> >              rSpec = ss->ssl3.crSpec;
> >          }
> > +        if (type == certificate_verify && ss->version == SSL_LIBRARY_VERSION_TLS_1_2) {
> > +            msgLenForCertVerify = ss->ssl3.hs.messages.len;
> 
> I was going to suggest that you move this to ssl3_ComputeHandshakeHashes()
> but I see that you are skipping the hashing for this message.  Why not tweak
> the value of computeHashes instead?

Good suggestion, done.


Regarding your comments:

> Where does ss->ssl3.hs.messages.len point to in the transcript?  (If you
> keep this code, you will need to include a comment explaining what you are
> doing; it's not obvious.)

and:

(In reply to Martin Thomson [:mt:] from comment #59)
> (In reply to Kai Engert (:kaie) from comment #57)
> > Because of this change, with TLS 1.2, I must pass along msgLenForCertVerify,
> > as explained in 55.
> 
> I'm not sure that you need this.  There's already a provision for messages
> that shouldn't be included in the hash, can you use that?  See my review for
> more.


I've added the following comment:

    /* We cannot compute the hash yet. We must wait until we have
     * decoded the certificate_verify message in
     * ssl3_HandleCertificateVerify, which will tell us which
     * hash function we must use.
     * We store the length of the handshake messages that we
     * have received until now and will pass it along,
     * because that's the amount that must be used when computing
     * the hash for certificate_verify.
     * (ssl3_HandleCertificateVerify cannot simply look at the
     * buffer length itself, because at the time we reach it,
     * additional handshake messages will have been added to the
     * buffer, e.g. the certificate_verify message itself.)
     */


I haven't yet found the "provision to exclude from the hash", but I suspect that cannot be used, because the certificate_verify must be included when computing later hashes.
Attached patch Patch v10 (obsolete) — — Splinter Review 
Also uploaded as patch v10 to https://codereview.appspot.com/300860043

This implements most of your requests, except as noted in my comments.

This is an intermediate patch, but I hope it can clarify, especially regarding msgLenForCertVerify.

I still have to work on Martin's very last comments, regarding the pick/decide functions.
Attachment #8758051 - Attachment is obsolete: true
Attachment #8758051 - Flags: review?(martin.thomson)
Re the last few comments, how about skipping ssl3_ComputeHandshakeHashes() for CertificateVerify, but then call it manually in ssl3_HandleCertificateVerify() so that you can control when the hashes get included?
unfortunately, the hashes are updated in the handshake handling code
Martin, did you see the following comment from ssl3_ExtractClientKeyInfo(), which explains why SHA1 is preferred in some scenarios? It seems that was done to ensure things work for people in Estonia with their government issued cards.

    /* If the key is a 1024-bit RSA or DSA key, assume conservatively that
     * it may be unable to sign SHA-256 hashes. This is the case for older
     * Estonian ID cards that have 1024-bit RSA keys. In FIPS 186-2 and
     * older, DSA key size is at most 1024 bits and the hash function must
     * be SHA-1.
     */
(In reply to Martin Thomson [:mt:] from comment #58)
> @@ +6865,5 @@
> >           * H is the negotiated signature hash and
> >           * Hash is the cipher-suite specific handshake hash
> >           * Generally this means that Hash is SHA-256.
> >           *
> > +         * We need code to negotiate H.
> 
> Maybe you can instead say that H is negotiated based on the
> CertificateRequest and that negotiation is done in
> ssl3_HandleCertificateRequest().


That comment refers to a TODO for TLS 1.3, and I don't fully understand what remains to be done for TLS 1.3, and I'd prefer if that could be handled in a follow-up.

All I did to that comment, I removed the mention of the backupHash variable, because it has been removed.

I assumed that Eric would follow up on this, after this one has landed, if anything remains to be done for TLS 1.3
Attached patch Patch v11 (obsolete) — — Splinter Review 
I've worked on merging functions ssl3_PickSignatureHashAlgorithm and ssl3_DecideTls12CertVerifyHash, but they are too different. I still have both entry points.

However, I moved the common loop to a shared function ssl3_FindBestHashAlgForSigAlg. That requires to convert one array or the other, because both entry points operate on different array types.

I've chosen to keep ssl3_PickSignatureHashAlgorithm as is, because it's executed more often, and do the conversion in the less frequently used function ssl3_DecideTls12CertVerifyHash.

Also, the double loop became more complicated, because it must derive the information, whether or not Sha1 is acceptable (in addition to the preference). I believe we must still use the Sha1-preference, which isn't a client side preference, but rather our decision to use it, when it's likely to be necessray to support Estonian smartcards.

This patch has 20 lines context. I'll also upload to rietveld at the above link.
Attachment #8758282 - Attachment is obsolete: true
Attachment #8758721 - Flags: review?(martin.thomson)

    
    

    
  
I'm trying to understand which further changes I must make before this can land.

I got some short feedback from Martin on IRC and on rietveld.

I believe what he said is:
(a) combining the ssl3_PickSignatureHashAlgorithm and ssl3_DecideTls12CertVerifyHash
    doesn't look good, and keeping them separate seems better.

=> I'll undo the attempt to merge them.

(b) take snapshots of the message buffer when it's necessary to compute a TLS 1.2
    handshake_hash_record, and only hash the snapshot when necessary.

This was said in response to the long comment I've added to the code, see end of comment 65 in this bug.

I fail to understand the benefit of that.

There are multiple occasions that require the hash to be calculated.

In most scenarios, there isn't a need for taking a snapshot, because the state of the queued buffer is exactly what we require as input for calculating the hash. It's simple as that when calculating the hash for the "finished" message, when computing the extended master secret, and it's simple when *sending* the the certicate_verify message.

There's only one scenario when it's required to take an earlier snapshot of the buffered handshake messages: When *receiving* (and handling) a certicate_verify message.

Because it's a simple logic in the general case, the idea of always taking snapshots (and copying buffers) seem like an unnecessary extra runtime cost.

I think it's worth it avoiding memory allocations. It's less expensive to pass along the buffer length that must be used for calculation, than copying a buffer, therefore I prefer the existing solution that uses the msgLenForCertVerify parameter.

    
  

        Attachment #8758721 -
        Flags: review?(martin.thomson)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v12 (obsolete)
        — 
          — Splinter Review
      

    


  
I've reverted the attempt to merge the pick/decide functions, therefore this patch is almost identical to patch v10.

Only difference is the inclusion of the correctness fix from comment 72.

Uploaded to rietveld:
https://codereview.appspot.com/300860043/#ps60001

Martin, do you think this is good for checkin?

Ideally, I'd like to like this get landed today, or with potential small fixes by tomorrow. Thanks.

        Attachment #8758721 -
        Attachment is obsolete: true

        Attachment #8758767 -
        Attachment is obsolete: true

        Attachment #8759119 -
        Flags: review?(martin.thomson)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1179338-v13-U20.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Addressed Martin's comments from rietveld.

        Attachment #8759119 -
        Attachment is obsolete: true

        Attachment #8759119 -
        Flags: review?(martin.thomson)

        Attachment #8759218 -
        Flags: review?(martin.thomson)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v14 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8759443 -
        Flags: review?(martin.thomson)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v16 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8759218 -
        Attachment is obsolete: true

        Attachment #8759443 -
        Attachment is obsolete: true

        Attachment #8759218 -
        Flags: review?(martin.thomson)

        Attachment #8759443 -
        Flags: review?(martin.thomson)

        Attachment #8759476 -
        Flags: review?(martin.thomson)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v17
        — 
          — Splinter Review
      

    


  

        Attachment #8759476 -
        Attachment is obsolete: true

        Attachment #8759476 -
        Flags: review?(martin.thomson)

        Attachment #8759479 -
        Flags: review?(martin.thomson)

    
    

    
  
Comment on attachment 8759479 [details] [diff] [review]
Patch v17

got r+ on rietveld, Martin, thanks a lot

        Attachment #8759479 -
        Flags: review?(martin.thomson) → review+

    
    

    
  
https://hg.mozilla.org/projects/nss/rev/151f2a8846d2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED

    
    

    
  
Thank you for fixing this. It resolves another situation we had with TLS 1.2 client auth, where IE7 was the client, connecting to an Oracle server using NSS. The server was only requesting 3 algorithms in the CertificateRequest : SHA256/RSA, SHA256/ECDSA or SHA256/DSA . And IE was responding with an empty Certificate message. This was particularly confusing, because IE was prompting the user to select the client cert, and after the user did so, IE proceeded to not send it over the wire, because that cert didn't match the server's supported algorithms. Downgrading to TLS 1.1 was the only workaround. It works as expected now, and appears to work correctly both for PKCS#11 bypass or non-bypass modes.

Things were working fine with Firefox and TLS 1.2 and the same client cert, because FF was ignoring the server's signature_algorithms and still sending the SHA-1 cert despite it not matching what was requested. IE was compliant with RFC, and FF was not. I have not tested a build of FF with NSS 3.25 yet - only a build of our servers.

I have still not made progress on our other case, where NSS was the client side, using PKCS#11 bypass, but expect to be able to do so soon, at least in terms of finally creating the test case.

    
    

    
  
Spoke too soon on bypass case describes in the previous post. Server actually asserts with the following stack:

#0  0x0000003cbd4325e5 in raise () from /lib64/libc.so.6
#1  0x0000003cbd433dc5 in abort () from /lib64/libc.so.6
#2  0x00007f6fdcef3b93 in PR_Assert (s=0x7f6fdcc6bb6a "0", file=0x7f6fdcc6b878 "ssl3con.c", ln=4028) at ../../../../pr/src/io/prlog.c:553
#3  0x00007f6fdcc292b3 in ssl3_GetHashMechanismByHashType (hashType=ssl_hash_sha1) at ssl3con.c:4028
#4  0x00007f6fdcc2b651 in ssl3_ComputeBypassHandshakeHash (buf=0x7f6fb8035f80 "\001", len=1500, hashAlg=ssl_hash_sha1, hashes=0x7f6fd04728b0)
    at ssl3con.c:5157
#5  0x00007f6fdcc377a2 in ssl3_HandleCertificateVerify (ss=0x7f6fb8021ff0, b=0x7f6fb802abe7 "", length=130, hashes=0x7f6fd04729f0) at ssl3con.c:10571
#6  0x00007f6fdcc3c457 in ssl3_HandlePostHelloHandshakeMessage (ss=0x7f6fb8021ff0, b=0x7f6fb802abe5 "\002\001", length=132, hashesPtr=0x7f6fd04729f0)
    at ssl3con.c:12750
#7  0x00007f6fdcc3c089 in ssl3_HandleHandshakeMessage (ss=0x7f6fb8021ff0, b=0x7f6fb802abe5 "\002\001", length=132, endOfRecord=1) at ssl3con.c:12657
#8  0x00007f6fdcc3c774 in ssl3_HandleHandshake (ss=0x7f6fb8021ff0, origBuf=0x7f6fb8022228) at ssl3con.c:12838
#9  0x00007f6fdcc3ddde in ssl3_HandleRecord (ss=0x7f6fb8021ff0, cText=0x7f6fd0472b90, databuf=0x7f6fb8022228) at ssl3con.c:13528
#10 0x00007f6fdcc3ff45 in ssl3_GatherCompleteHandshake (ss=0x7f6fb8021ff0, flags=0) at ssl3gthr.c:480
#11 0x00007f6fdcc40b9e in ssl_GatherRecord1stHandshake (ss=0x7f6fb8021ff0) at sslcon.c:81
#12 0x00007f6fdcc4997d in ssl_Do1stHandshake (ss=0x7f6fb8021ff0) at sslsecur.c:70
#13 0x00007f6fdcc4b8c5 in ssl_SecureRecv (ss=0x7f6fb8021ff0, buf=0x7f6fb80009b8 "", len=8191, flags=0) at sslsecur.c:856
#14 0x00007f6fdcc55acc in ssl_Recv (fd=0x7f6fa4008d70, buf=0x7f6fb80009b8, len=8191, flags=0, timeout=20000) at sslsock.c:2698
#15 0x00007f6fdceef994 in PR_Recv (fd=0x7f6fa4008d70, buf=0x7f6fb80009b8, amount=8191, flags=0, timeout=20000) at ../../../../pr/src/io/priometh.c:188

However, the server auto-restarts itself, and somehow, IE reinitiates the handshake, and the subsequent handshake doesn't hit this assertion, and IE is then able to send the client cert to the web server, which successfully validates the credentials, and serves the page. Not sure what would happen in an optimized NSS build yet in this situation - have not tried it. But the inconsistent behavior of the assertion is pretty strange. So , it looks like bypass still needs to be disabled for this to work right.

    
    

    
  
Rather than hijack this bug, I filed bug 1280348 to deal with the assertion in the bypass case. A patch is attached.

    
    

    
  
I was finally able to create a test case for the case where the NSS client was responding with a SHA-256 hash in CertVerify and the server was rejecting it. I did this by making a manually code change to the server side to do the following to force it to accept only SHA-1/RSA, per Erik's suggestion :

    SSLSignatureAndHashAlg alg;
    alg.hashAlg = ssl_hash_sha1;
    alg.sigAlg = ssl_sign_rsa;
    stat = SSL_SignaturePrefSet(sock, &alg, 1);

I also configured the server to require client auth in the first handshake.

Then, using an NSS client (command-line tool), and a cipher suite that uses SHA-1, I ran a test that sent the SHA-1/RSA client cert.
The handshake failed on both sides (in the server first) when both sides were using NSS 3.21. 
Once I put NSS 3.25 on the client side, which has this fix, the handshake succeeded.
It still succeeds even if the client side uses PKCS#11 bypass mode. So, it looks like the fix for this bug worked for both the bypass and PKCS#11 cases. Thank you.
Depends on: CVE-2017-7805

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: