Yash Kumar Saini

Posted on Aug 20

The test was green. Every real connection would have failed.

#devchallenge #bugsmash #python #testing

Summer Bug Smash: Smash Stories 🐛🛹

This is a submission for DEV's Summer Bug Smash: Smash Stories powered by Sentry.

The setting

WebRTC-Direct in libp2p has a neat trick for connecting without a certificate authority: the peer's multiaddr contains a hash of its TLS certificate — /webrtc-direct/certhash/<...>. When you dial, the DTLS handshake presents a cert, you hash it, and you check it against the certhash in the address. No CA, no trust store — the address is the pin.

For that to work, py-libp2p has to make aiortc use our libp2p-generated certificate, not the one aiortc auto-generates for itself. So the code pins it:

config = RTCConfiguration(certificates=[rtc_cert])
return RTCPeerConnection(configuration=config)

Clean. Obvious. And, as of aiortc ≥ 1.5, completely wrong — in two different ways.

The first sign

The first way was loud: aiortc ≥ 1.5 dropped certificates= from RTCConfiguration. Passing it raises TypeError. Easy to spot, easy to "fix" — just move the cert onto the object after construction:

pc = RTCPeerConnection(configuration=config)
pc._certificates = [rtc_cert]   # looks right. isn't.
return pc

The tests went green. The loopback echo test — open a data channel, send a payload, get it back — passed. I could have shipped it right there.

The quiet way is the one that would have burned every user.

The investigation

Here's the thing that made this a story and not a one-liner. pc._certificates = [...] is a silent no-op. aiortc never reads _certificates. Inside the class, it stores and reads the cert as self.__certificates — a double-underscore attribute. And Python does something specific with double-underscore names inside a class body: it mangles them. self.__certificates inside class RTCPeerConnection is compiled to self._RTCPeerConnection__certificates.

So from the outside:

pc._certificates — a brand-new attribute I invented. aiortc reads it never.
pc._RTCPeerConnection__certificates — the actual slot aiortc reads at createOffer/createAnswer time to write the SDP a=fingerprint line (see aiortc/rtcpeerconnection.py:295 and :1129).

I was setting the first one. aiortc was reading the second one — still holding its own auto-generated cert. The result:

The /webrtc-direct/certhash/<...> multiaddr advertised our libp2p cert hash.
The DTLS handshake used aiortc's auto-generated cert.
Every real dial would fail peer verification with Remote DTLS fingerprint does not match certhash.

And the loopback echo test still passed — because loopback doesn't validate the DTLS fingerprint against a multiaddr. It just opens a channel and echoes bytes. The one property that was broken was the one property the happy-path test never checked.

The root cause

This is a bug that lives in the seam between two systems:

Python's language rule: name mangling rewrites __name references inside a class body so subclasses can't accidentally clobber a base class's "private" attributes. It's a language feature doing exactly what it's designed to do.
aiortc's library internals: it stores the cert under __certificates, i.e. it relies on that mangling for its own encapsulation.

Neither is wrong on its own. But when you reach into a library's private state from outside the class, the mangled name is the only name that works — and the un-mangled one you'd naturally type is a no-op that fails silently. There's no AttributeError, no TypeError, no warning. You just create a junk attribute and move on, and everything downstream keeps working except the security property.

The fix

Set the mangled attribute directly, after construction, before any SDP operation triggers the handshake:

config = RTCConfiguration(iceServers=list(ice_servers) if ice_servers else [])
pc = RTCPeerConnection(configuration=config)
# Replace aiortc's auto-generated cert. Must use the mangled name —
# aiortc reads only `self.__certificates`, which mangles to this.
pc._RTCPeerConnection__certificates = [rtc_cert]  # type: ignore[attr-defined]
return pc

It's an ugly line, and the docstring says so out loud — reaching into _RTCPeerConnection__certificates is exactly the kind of private-state coupling that breaks on a library upgrade. But it's correct, and it's annotated so the next person knows why it can't just be pc._certificates.

The canary that would have caught it

The real fix wasn't the one-liner — it was the test that makes the silent failure loud. The happy-path echo test can't see this bug, so I added one that asserts the invariant: the pinned cert's fingerprint must actually appear in the SDP.

@pytest.mark.trio
async def test_cert_pinning_lands_in_sdp_fingerprint() -> None:
    """
    Guards against a class of cert-pin bug: if the pin is a no-op (e.g.
    written to a public attribute aiortc never reads because it actually
    stores the cert under a name-mangled private slot), the SDP a=fingerprint
    line reflects aiortc's auto-generated cert instead of ours.
    """
    cert = WebRTCCertificate.from_aiortc()
    pc = await bridge.run_coro(create_peer_connection(cert._rtc_certificate, ice_servers=[]))

    # Need at least one data channel for the SCTP m-line, otherwise
    # createOffer omits the DTLS fingerprint entirely.
    await bridge.run_coro(_create_dummy_channel(pc))

    sdp = await bridge.run_coro(_offer())
    expected = _sdp_fingerprint_string(cert)
    assert expected in sdp.upper(), "SDP fingerprint does not match pinned cert."

Note the small landmine inside the fix for the test: createOffer omits the DTLS fingerprint entirely unless there's at least one SCTP m-line, so the canary has to create a dummy data channel first or it asserts against an empty offer. The bug had layers even in the reproduction.

Revert the mangled-name fix and this test goes red immediately with a fingerprint mismatch. Keep the old pc._certificates = [...] no-op and it's red. That's the whole point: it fails closed.

What I learned

Two lessons, and they're the same lesson from two directions.

Test the invariant, not the happy path. The echo test proved bytes moved. It said nothing about which certificate moved them — and that was the only thing that mattered for security. A green test is only as good as the property it asserts, and "it worked end to end" quietly excludes every property your end-to-end path doesn't check.

A silent no-op is worse than a crash. If pc._certificates = [...] had raised, I'd have fixed it in thirty seconds. Because it succeeded — created a real attribute, threw no error — it sailed through review and tests and would have shipped a transport where every authenticated dial fails. When you reach across an encapsulation boundary into a library's private state, assume the language is doing something clever with the name, and write the test that proves your write actually landed where the library reads.

DEV Community