Third-Party Audit of Rustls

Some quotes from the report's conclusions:

"the team of auditors considered the general code quality really good and can attest to a solid impression left consistently by all scope items."

"was consistently well-documented and readable, demonstrating that security processes are ingrained in the development and documentation processes"

"Both from a design point of view as from an implementation perspective the entire scope can be considered of exceptionally high standard (...) no directly exploitable weaknesses could be identified."

"It appears to have been developed with all previously known issue-types in mind; furthermore, its missing support for insecure or outdated protocols and primitives indicates a security-conscious development approach."

"the developers of rustls have an extensive knowledge on how to correctly implement the TLS stack whilst avoiding the common pitfalls that surround the TLS ecosystem. This knowledge has translated reliably into an implementation of exceptional quality."

"The developer’s intent to provide a high-quality TLS implementation is very clear and this goal can be considered as achieved successfully. (...) Cure53 had the rare pleasure of being incredibly impressed with the presented software."

I especially liked the: "We couldn't reason about this. So we rewrote it."

Thankyouthankyouthankyouthankyou.

I have this discussion all the time and I can't get people to see the issue.

> TLS-01-004 - Data Truncation in DER Encoding Implementation (low). This finding rightly points out a function in rustls that produces incorrect output when applied to an X.501 Name that is larger than 64KB. While that’s an exceedingly unlikely case, and the bug does not cause unsafe operation (but perhaps connection failure), the function has been corrected to produce valid output for all inputs.

The ISO 7816 smartcard standard uses this same ASN.1-based scheme for command and reply encoding. I found a memmove overflow and a separate off-by-one bug in Yubico's PC/SC library, and an off-by-one bug in the Yubikey 4 firmware, all related to their handling of this encoding.

It's possible that the Yubikey 4 bug could have been leveraged into an information leak, a buffer overflow, or some other exploit of the firmware.[1] The firmware was calculating a command reply length based on what the proper, compact length of the object should be, not the actual length of the stored object with malformed internal encoding, leading to corrupt reply chains. But I never explored it. I'm not an exploit developer, and didn't have time to go down that rabbit hole. I only discovered the bugs tracking down a bug in my own PC/SC library. It turned out I was generating an overlong encoding ({0x82 0x00 0xff} instead of {0x81 0xff}) for an object, tickling the Yubikey 4 issue. Yubico quickly fixed their library, but the engineer resolving the ticket downplayed the firmware issue, which seemed far more concerning as it suggested bug-prone buffer management techniques in the firmware.[2] (Indeed, it suggested that the firmware reused the buggy library code.) I haven't gone back to see what happened with the firmware since then.

Anyhow, paying attention to small encoding and decoding issues like these are important--the details always matter. "This couldn't possibly lead to an exploit" are famous last words.

[1] Hopefully not a leak of the key itself as I presume it was inaccessible to the firmware, but who knows--it's a black box.

[2] IIRC, their argument was garbage in, garbage out (GIGO), which is a dangerous misapplication of that principle. A malformed object should never result in malformed, corrupt responses by other layers of the protocol stack. In fact, the GIGO principle usually implies the opposite--that the behavior of other layers of the stack is invariant to bad data, not conditioned by it.

EDIT: Looking at my Git logs it seems my code was never buggy. My library couldn't read back some certificates generated by another, third, project and written to the card using the Yubico SDK. That third project was generating the over long encoding, and in the process of tracking down the root of the issue I stumbled upon the Yubico bugs. IIRC, that third project was never reading back the certificate as they had a very convoluted scheme for 2FA and client-side SSH X.509 certificates that relied heavily on the bespoke behavior of their client-side Go daemon.

This is really exciting! Many popular Rust libraries support switching between rustls and openssl for their TLS needs and it’s much easier to do cross-platform builds with rustls.

> After spending thirty days on the scope in late May and early June of 2020, the team of auditors considered the general code quality really good and can attest to a solid impression left consistently by all scope items. Naturally, this is partially thanks to the usage of Rust as the preferred language for the entire implementation of the rustls project.

The finding in TLS-01-003 is surprising to me, mostly because it presupposes a lack of sophistication among users of this library who are also using X.509 NameConstraints. From RFC5280:

  For example, a name constraint for "class C" subnet 192.0.2.0 is represented as the octets C0 00 02 00 FF FF FF 00, representing the CIDR notation 192.0.2.0/24 (mask 255.255.255.0).

As they mention in the findings:

  Typically, subnet masks should be contiguous and the presence of a non-contiguous mask might indicate a typo (such as 225.255.255.0 vs. 255.255.255.0), or potentially an attempt to bypass an access control scheme. Therefore, it is recommended to treat certificates containing non-contiguous subnet masks in their name constraints as invalid.

This seems to run counter to the intent in the RFC. By allowing for a four-octet subnet mask, instead of simply an int to represent the a contiguous CIDR mask, the RFC authors may have intended that more complex IP-based NameConstraints could be constructed. This certainly would make a huge difference for something like an intermediate (CA:TRUE), where it becomes much more economical to specify a sparse mask for a highly templated network. Think certs for network equipment or VoIP phones with regular, repeatable IP addressing across many locations/networks. E.g., a VoIP provisioning system that has an intermediate issuing CA with the following NameConstraints: IP:172.16.0.0/255.255.1.239.

If any change comes from this specific finding, I would hope that it's simply a flag to allow or disallow the use of discontiguous masks. I do understand that this is specific to WebPKI; having said that, if a client is implemented using rustls (with these recommendations enabled) and it happens across a perfectly valid certificate issued by an intermediate with a discontiguous mask in the NameConstraints, presumably it would fail or otherwise break. And yes, I have previously configured precisely this in an intermediate CA.

"There were two informational and two minor-severity findings."

They found a few small issues but nothing horrible

I wonder, with deprecation of TLS 1.0 and 1.1 support being planned, whether Firefox can adopt rustls in favor of NSS.