Security code review/audit of PKI.js library
Categories
(Firefox Graveyard :: Security: Review Requests, task)
Tracking
(Not tracked)
People
(Reporter: dveditz, Unassigned)
References
()
Details
As part of a project to re-implement our certificate viewer (along the line of April's "Certainly Something" extension) the team would like to use the PKI.js crypto library. Appears to be built on top of WebCrypto so the cryptography primitives should be good, though there's a a polyfill for implementations that don't support WebCrypto. We'll want to make sure we're not falling into that path accidentally.
The point of contacts would be Carolina Jimenez Gomez (cc'd) and Danielle Leblanc-Cyr (couldn't find bugmail in phonebook).
The library is https://github.com/PeculiarVentures/PKI.js and it's been approved for use in FIrefox in bug 1552942.
|
||
Comment 1•5 years ago
•
|
||
(In reply to Daniel Veditz [:dveditz] from comment #0)
The point of contacts would be Carolina Jimenez Gomez (cc'd) and Danielle Leblanc-Cyr (couldn't find bugmail in phonebook).
[Edit] CC'ed Danielle
|
||
Comment 2•5 years ago
|
||
I should clarify our usage of this library:
- This new cryptography library is for display only. It will never be used by Firefox for verifying certificates or making any other security or trust decision. Of course, the displayed content will be used by users for trust decisions, though IMO that's a significantly lower value target for potential attacks, since it's not displayed in primary UI or anything.
We had discussed the pros and cons of using PKI.js vs. extending our own internal functionalities and the CryptoEng team specifically is interested in the idea of "outsourcing" display-related code to be separate to the code that does verification.
- The library will be run with content privileges, not system privileges. The entire page it runs on has content privileges and very limited additional functionality (it can request the certificate for a site from the parent process). The page has a CSP, naturally.
Thus, 1) makes me believe that we only need very basic (if at all) review of the cryptographic primitives here.
What I'm mostly interested in is classic security aspects of the JS code base. Are they using insecure patterns such as eval? Do they pull in any third party sources? How do they handle security issues? Again, all considering that the library will have very little powers as of 2).
Thanks!
(In reply to Johann Hofmann [:johannh] from comment #2)
I should clarify our usage of this library:
- This new cryptography library is for display only. It will never be used by Firefox for verifying certificates or making any other security or trust decision. Of course, the displayed content will be used by users for trust decisions, though IMO that's a significantly lower value target for potential attacks, since it's not displayed in primary UI or anything.
We had discussed the pros and cons of using PKI.js vs. extending our own internal functionalities and the CryptoEng team specifically is interested in the idea of "outsourcing" display-related code to be separate to the code that does verification.
- The library will be run with content privileges, not system privileges. The entire page it runs on has content privileges and very limited additional functionality (it can request the certificate for a site from the parent process). The page has a CSP, naturally.
Thus, 1) makes me believe that we only need very basic (if at all) review of the cryptographic primitives here.
What I'm mostly interested in is classic security aspects of the JS code base. Are they using insecure patterns such as eval? Do they pull in any third party sources? How do they handle security issues? Again, all considering that the library will have very little powers as of 2).
Thanks!
Thanks for the info. Yeh I agree with you that the risk of code injection is pretty low (the lib would have to be really bad, and even then CSP saves us). But I can take a look at the things you ask for above.
The only risk I can think of is the one you highlight (1) above, ie that differences in cert parsing allow for spoofing. As you say, its not the primary place for trust decisions. Still I wouldn't be great if a cert could exactly spoof another legitimate site. How's the unicode handling? (e.g. try https://xn--jv8h.xn--vsb.com/ - just tested and this crashes fenix... /me goes to file another bug....)
|
|
||
Comment 4•5 years ago
|
||
Any updates on this? We would like to ship with Firefox 71 and have had about:certificate running in Nightly for a while now.
|
||
Comment 5•5 years ago
|
||
FWIW, I've audited this when it was still an add-on and just reconfirmed that XSS-like risks are mitigated. The code makes use of textContent and checks URLs to start with http before inserting a elements (to prevent javascript URLs).
I've also created a cert with XSS-like stuff that I can attach to the bug if folks are interested, but it really just takes a hex editor to replace common names or URLs with javascript:.. or HTML elements like <s>. I also can't reproduce the problem in comment 3. Both certs parse fine.
The remaining risk is what this bug was originally filed for. Someone comes up with a clever certificate that when decoded from our C++ parser says domainA and the cert viewer says it's domainB. I'd argue that this does not need to be tested with high priority. This is not on the security-critical path, but rather a correctness problem (no trust decisions are made in the cert viewer!).
Moving from Blocks to See Also.
|
|
||
Comment 6•5 years ago
|
||
Awesome, thank you Freddy!
|
|
||
Comment 7•5 years ago
|
||
(I'd be interested in that test certificate)
Thanks freddy, closing since I don't think there is anything more to do here. For good measure I had a look at the code again myself and this morning. One thing I specifically looked at was how we create links in about:certificates, but there seems to be strong enough checks here [1]
|
||
Updated•4 years ago
|
Description
•