"Critical" severity shows up from Dependency Scan

[muralisrini]

When running Dependency Scan we see Critical severities related to CVE-2020-26892.

Turns out this stems from the "github.com/cloudflare/cfssl" library. There seems to be only a single use of the library via github.com/cloudflare/cfssl/revoke (in the github.com/duo-labs/webauthn/metadata package) and that API does not use the function that raises the CVE.

Given that its the lone use, implementing that fairly small code (in relation to the library) directly into webauthn and removing "github.com/cloudflare/cfssl" would get rid of the Critical severity (and some "High" severities as well) seems like a good option (the other option would be to fix the library which might be more involved).

Is that a PR you'd consider ?

[james-d-elliott]

I can't really talk on behalf of the project but while dependency scanning tools are great at identifying potential issues, they nearly always require more investigation. Unless the CVE exists directly in the library in question, it is more often than not a CVE that doesn't affect the specific usage.

My guess on the CVE coming up is that it doesn't affect this lib based on the description. However I personally dislike the cfssl library for various reasons and think a majority of the usage will be easy to implement other than OCSP due to golang/go#40017.

[james-d-elliott]

Looks like there is some preliminary support in golang.org/x/crypto/ocsp?

Edit: So looking at the library, the CRL stuff is easy to implement other than the CRL cache, OCSP is probably the only hard thing. Apparently the issue with the golang.org/x/crypto/ocsp implementation is it only handles a minor portion of the tasks involved.

[muralisrini]

My guess on the CVE coming up is that it doesn't affect this lib based on the description.

Yes, as far as I could tell, it doesn't affect this lib. Just that it'll be nice not to have to haven an exception or explain it to scanners.

[muralisrini]

Looks like there is some preliminary support in golang.org/x/crypto/ocsp?

Edit: So looking at the library, the CRL stuff is easy to implement other than the CRL cache, OCSP is probably the only hard thing. Apparently the issue with the golang.org/x/crypto/ocsp implementation is it only handles a minor portion of the tasks involved.

I bumped into golang.org/x/crypto/ocsp as well but couldn't quite tell if it can be used to replace the cfssl based code.

[james-d-elliott]

Looks like there is some preliminary support in golang.org/x/crypto/ocsp?
Edit: So looking at the library, the CRL stuff is easy to implement other than the CRL cache, OCSP is probably the only hard thing. Apparently the issue with the golang.org/x/crypto/ocsp implementation is it only handles a minor portion of the tasks involved.

I bumped into golang.org/x/crypto/ocsp as well but couldn't quite tell if it can be used to replace the cfssl based code.

It looks like it can craft the request and parse responses, but it doesn't specifically send/receive them. Which basically means most of the heavy lifting is done for you.

Regarding Cloudflare SSL.. the specific areas we utilize could be vastly improved honestly. If you look at how it parses CRL's the issuer is retrieved when it may not be needed and far before it is needed. Additionally there are ineffectual uses of the mutex (i.e. when a CRL shouldn't be updated it's used twice when it can be used once).

[muralisrini]

Looks like there is some preliminary support in golang.org/x/crypto/ocsp?
Edit: So looking at the library, the CRL stuff is easy to implement other than the CRL cache, OCSP is probably the only hard thing. Apparently the issue with the golang.org/x/crypto/ocsp implementation is it only handles a minor portion of the tasks involved.

I bumped into golang.org/x/crypto/ocsp as well but couldn't quite tell if it can be used to replace the cfssl based code.

It looks like it can craft the request and parse responses, but it doesn't specifically send/receive them. Which basically means most of the heavy lifting is done for you.

Looking more closely, see the revCheck func in revoke.go treats both CRL and OCSP and in fact uses golang.org/x/crypto/ocsp for the later. Missed that in the first reading. Not sure if that observation changes anything you mention above ?

Regarding Cloudflare SSL.. the specific areas we utilize could be vastly improved honestly. If you look at how it parses CRL's the issuer is retrieved when it may not be needed and far before it is needed. Additionally there are ineffectual uses of the mutex (i.e. when a CRL shouldn't be updated it's used twice when it can be used once).

Agree that getIssuer call is at the wrong place. I didn't get the comment on the mutex though. Looks like the two paired Lock/Unlock cannot be reduced to one usage in any flow assuming the API can be called concurrently ? Am I missing something ?

Edit: More broadly, if are going the replacement route, is the suggestion to reimplement VerifyCertificate using the Cloudflare SSL code with the indicated cleanup directly in webauthn ?

[james-d-elliott]

I didn't look to closely. Likely can implement it. Another potential issue is PKCS #7 which it handles, but I don't think stdlib has anything for. Mozilla has another library that handles PKCS #7 parsing I believe if it's necessary.

Agree that getIssuer call is at the wrong place. I didn't get the comment on the mutex though. Looks like the two paired Lock/Unlock cannot be reduced to one usage in any flow assuming the API can be called concurrently ? Am I missing something ?

Edit: More broadly, if are going the replacement route, is the suggestion to reimplement VerifyCertificate using the Cloudflare SSL code with the indicated cleanup directly in webauthn ?

Oh never mind, on second reading it is fine. I thought they arbitrarily deleted it from the map and added it back but that's only if it's nil.

Edit: More broadly, if are going the replacement route, is the suggestion to reimplement VerifyCertificate using the Cloudflare SSL code with the indicated cleanup directly in webauthn ?

I think Nick or one of the other guys would have to comment on this. I think the most ideal solution is finding a library that already does it but is maintained and not a giant mess. Though what you propose is probably the only alternative to leaving it as is at present.