From a05e6818b3155d97e92bb6a04eb6bfc9a8d6c881 Mon Sep 17 00:00:00 2001 From: Jonathan Lennox Date: Tue, 26 Nov 2024 11:13:35 -0500 Subject: [PATCH] Update fingerprint handling code for RFC 8122 conformance. (#2254) --- .../kotlin/org/jitsi/nlj/dtls/DtlsConfig.kt | 28 +++++++ .../kotlin/org/jitsi/nlj/dtls/DtlsStack.kt | 2 +- .../kotlin/org/jitsi/nlj/dtls/DtlsUtils.kt | 83 ++++++------------- .../src/main/resources/reference.conf | 5 ++ .../org/jitsi/nlj/dtls/DtlsConfigTest.kt | 54 ++++++++++++ .../kotlin/org/jitsi/nlj/dtls/DtlsTest.kt | 4 +- .../kotlin/org/jitsi/videobridge/Endpoint.kt | 5 +- .../org/jitsi/videobridge/relay/Relay.kt | 5 +- .../transport/dtls/DtlsTransport.kt | 2 +- 9 files changed, 122 insertions(+), 66 deletions(-) diff --git a/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsConfig.kt b/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsConfig.kt index 6764aa94f8..40f689da7e 100644 --- a/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsConfig.kt +++ b/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsConfig.kt @@ -15,6 +15,7 @@ */ package org.jitsi.nlj.dtls +import org.bouncycastle.operator.DefaultDigestAlgorithmIdentifierFinder import org.bouncycastle.tls.CipherSuite import org.jitsi.config.JitsiConfig import org.jitsi.metaconfig.ConfigException @@ -36,11 +37,38 @@ class DtlsConfig { } } + val localFingerprintHashFunction: String by config { + "jmt.dtls.local-fingerprint-hash-function".from(JitsiConfig.newConfig).transformedBy { + validateHashFunction(it) + } + } + + val acceptedFingerprintHashFunctions: List by config { + "jmt.dtls.accepted-fingerprint-hash-functions".from(JitsiConfig.newConfig).convertFrom> { list -> + if (list.isEmpty()) { + throw ConfigException.UnableToRetrieve.ConditionNotMet( + "accepted-fingerprint-hash-functions must not be empty" + ) + } + list.map { validateHashFunction(it) } + } + } + companion object { val config = DtlsConfig() } } +private fun validateHashFunction(func: String): String { + val ucFunc = func.uppercase() + DefaultDigestAlgorithmIdentifierFinder().find(ucFunc) + ?: throw ConfigException.UnableToRetrieve.WrongType("Unknown hash function $func") + if (ucFunc == "MD5" || ucFunc == "MD2") { + throw ConfigException.UnableToRetrieve.WrongType("Forbidden hash function $func") + } + return func.lowercase() +} + private fun String.toBcCipherSuite(): Int = try { CipherSuite::class.java.getDeclaredField(this).getInt(null) } catch (e: Exception) { diff --git a/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsStack.kt b/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsStack.kt index 4d7c32ba5c..14f0b13a82 100644 --- a/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsStack.kt +++ b/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsStack.kt @@ -71,7 +71,7 @@ class DtlsStack( /** * The remote fingerprints sent to us over the signaling path. */ - var remoteFingerprints: Map = HashMap() + var remoteFingerprints: Map> = mapOf() /** * A handler which will be invoked when DTLS application data is received diff --git a/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsUtils.kt b/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsUtils.kt index c1259c3ba1..6915fa3d04 100644 --- a/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsUtils.kt +++ b/jitsi-media-transform/src/main/kotlin/org/jitsi/nlj/dtls/DtlsUtils.kt @@ -65,7 +65,7 @@ class DtlsUtils { val cn = generateCN("TODO-APP-NAME", "TODO-APP-VERSION") val keyPair = generateEcKeyPair() val x509certificate = generateCertificate(cn, keyPair) - val localFingerprintHashFunction = x509certificate.getHashFunction() + val localFingerprintHashFunction = config.localFingerprintHashFunction val localFingerprint = x509certificate.getFingerprint(localFingerprintHashFunction) val certificate = org.bouncycastle.tls.Certificate( @@ -158,7 +158,7 @@ class DtlsUtils { */ fun verifyAndValidateCertificate( certificateInfo: org.bouncycastle.tls.Certificate, - remoteFingerprints: Map + remoteFingerprints: Map> ) { if (certificateInfo.certificateList.isEmpty()) { throw DtlsException("No remote fingerprints.") @@ -179,66 +179,33 @@ class DtlsUtils { * and validate against the fingerprints presented by the remote endpoint * via the signaling path. */ - private fun verifyAndValidateCertificate(certificate: Certificate, remoteFingerprints: Map) { - // RFC 4572 "Connection-Oriented Media Transport over the Transport - // Layer Security (TLS) Protocol in the Session Description Protocol - // (SDP)" defines that "[a] certificate fingerprint MUST be computed - // using the same one-way hash function as is used in the certificate's - // signature algorithm." - - val hashFunction = certificate.getHashFunction() - - // As RFC 5763 "Framework for Establishing a Secure Real-time Transport - // Protocol (SRTP) Security Context Using Datagram Transport Layer - // Security (DTLS)" states, "the certificate presented during the DTLS - // handshake MUST match the fingerprint exchanged via the signaling path - // in the SDP." - val remoteFingerprint = remoteFingerprints[hashFunction] ?: throw DtlsException( - "No fingerprint declared over the signaling path with hash function: $hashFunction" - ) - - // TODO(boris) check if the below is still true, and re-introduce the hack if it is. - // Unfortunately, Firefox does not comply with RFC 5763 at the time - // of this writing. Its certificate uses SHA-1 and it sends a - // fingerprint computed with SHA-256. We could, of course, wait for - // Mozilla to make Firefox compliant. However, we would like to - // support Firefox in the meantime. That is why we will allow the - // fingerprint to "upgrade" the hash function of the certificate - // much like SHA-256 is an "upgrade" of SHA-1. - /* - if (remoteFingerprint == null) - { - val hashFunctionUpgrade = findHashFunctionUpgrade(hashFunction, remoteFingerprints) - - if (hashFunctionUpgrade != null - && !hashFunctionUpgrade.equalsIgnoreCase(hashFunction)) { - fingerprint = fingerprints[hashFunctionUpgrade] - if (fingerprint != null) - hashFunction = hashFunctionUpgrade - } - } + private fun verifyAndValidateCertificate( + certificate: Certificate, + remoteFingerprints: Map> + ) { + /** RFC 8122: + * An endpoint MUST select the set of fingerprints that use its most + * preferred hash function (out of those offered by the peer) and verify + * that each certificate used matches one fingerprint out of that set. */ + config.acceptedFingerprintHashFunctions.forEach { hashFunction -> + val fingerprints = remoteFingerprints[hashFunction] ?: return@forEach - val certificateFingerprint = certificate.getFingerprint(hashFunction) + val certificateFingerprint = certificate.getFingerprint(hashFunction) - if (remoteFingerprint != certificateFingerprint) { - throw DtlsException( - "Fingerprint $remoteFingerprint does not match the $hashFunction-hashed " + - "certificate $certificateFingerprint" - ) + if (fingerprints.none { it == certificateFingerprint }) { + throw DtlsException( + "None of the fingerprints ${fingerprints.joinToString()} match the $hashFunction-hashed " + + "certificate $certificateFingerprint" + ) + } + return@verifyAndValidateCertificate } - } - - /** - * Determine and return the hash function (as a [String]) used by this certificate - */ - private fun Certificate.getHashFunction(): String { - val digAlgId = DefaultDigestAlgorithmIdentifierFinder().find(signatureAlgorithm) - - return BcDefaultDigestProvider.INSTANCE - .get(digAlgId) - .algorithmName - .lowercase() + /* If we got here none of our accepted fingerprint functions were listed. */ + throw DtlsException( + "No fingerprint declared over the signaling path with any of the accepted hash functions: " + + config.acceptedFingerprintHashFunctions.joinToString() + ) } /** diff --git a/jitsi-media-transform/src/main/resources/reference.conf b/jitsi-media-transform/src/main/resources/reference.conf index 52ee7c5e21..9fc672b7a0 100644 --- a/jitsi-media-transform/src/main/resources/reference.conf +++ b/jitsi-media-transform/src/main/resources/reference.conf @@ -43,6 +43,11 @@ jmt { // TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, // TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 ] + // The hash function to use to generate certificate fingerprints + local-fingerprint-hash-function = sha-256 + // The hash functions that are accepted for remote certificate fingerprints, in decreasing strength order + accepted-fingerprint-hash-functions = [ sha-512, sha-384, sha-256, sha-1 ] + } srtp { // The maximum number of packets that can be discarded early (without going through the SRTP stack for diff --git a/jitsi-media-transform/src/test/kotlin/org/jitsi/nlj/dtls/DtlsConfigTest.kt b/jitsi-media-transform/src/test/kotlin/org/jitsi/nlj/dtls/DtlsConfigTest.kt index 0029ffd101..b0b9ad900b 100644 --- a/jitsi-media-transform/src/test/kotlin/org/jitsi/nlj/dtls/DtlsConfigTest.kt +++ b/jitsi-media-transform/src/test/kotlin/org/jitsi/nlj/dtls/DtlsConfigTest.kt @@ -17,6 +17,7 @@ package org.jitsi.nlj.dtls import io.kotest.assertions.throwables.shouldThrow import io.kotest.core.spec.style.ShouldSpec +import io.kotest.matchers.collections.shouldContainExactly import io.kotest.matchers.shouldBe import org.bouncycastle.tls.CipherSuite import org.jitsi.config.withNewConfig @@ -64,5 +65,58 @@ class DtlsConfigTest : ShouldSpec() { } } } + context("Valid fingerprint hash functions") { + withNewConfig( + """ + jmt.dtls.local-fingerprint-hash-function = sha-512 + jmt.dtls.accepted-fingerprint-hash-functions = [ sha-512, sha-384, sha-256 ] + """.trimIndent() + ) { + DtlsConfig.config.localFingerprintHashFunction shouldBe "sha-512" + DtlsConfig.config.acceptedFingerprintHashFunctions shouldContainExactly + setOf("sha-512", "sha-384", "sha-256") + } + context("With inconsistent capitalization") { + withNewConfig( + """ + jmt.dtls.local-fingerprint-hash-function = SHA-512 + jmt.dtls.accepted-fingerprint-hash-functions = [ Sha-512, sHa-384, shA-256 ] + """.trimIndent() + ) { + DtlsConfig.config.localFingerprintHashFunction shouldBe "sha-512" + DtlsConfig.config.acceptedFingerprintHashFunctions shouldContainExactly + setOf("sha-512", "sha-384", "sha-256") + } + } + } + context("Invalid local fingerprint hash function") { + context("Invalid name") { + withNewConfig("jmt.dtls.local-fingerprint-hash-function = sha-257") { + shouldThrow { DtlsConfig.config.localFingerprintHashFunction } + } + } + context("Forbidden function") { + withNewConfig("jmt.dtls.local-fingerprint-hash-function = md5") { + shouldThrow { DtlsConfig.config.localFingerprintHashFunction } + } + } + } + context("Invalid accepted accepted fingerprint hash functions") { + context("Invalid entry") { + withNewConfig("jmt.dtls.accepted-fingerprint-hash-functions = [ sha-256, sha-257 ]") { + shouldThrow { DtlsConfig.config.acceptedFingerprintHashFunctions } + } + } + context("Empty") { + withNewConfig("jmt.dtls.accepted-fingerprint-hash-functions = []") { + shouldThrow { DtlsConfig.config.acceptedFingerprintHashFunctions } + } + } + context("Forbidden function") { + withNewConfig("jmt.dtls.accepted-fingerprint-hash-functions = [ sha-1, md5 ]") { + shouldThrow { DtlsConfig.config.acceptedFingerprintHashFunctions } + } + } + } } } diff --git a/jitsi-media-transform/src/test/kotlin/org/jitsi/nlj/dtls/DtlsTest.kt b/jitsi-media-transform/src/test/kotlin/org/jitsi/nlj/dtls/DtlsTest.kt index 2b0d9db85e..82bc4601fa 100644 --- a/jitsi-media-transform/src/test/kotlin/org/jitsi/nlj/dtls/DtlsTest.kt +++ b/jitsi-media-transform/src/test/kotlin/org/jitsi/nlj/dtls/DtlsTest.kt @@ -48,10 +48,10 @@ class DtlsTest : ShouldSpec() { val pcapWriter = if (pcapEnabled) PcapWriter(logger, "/tmp/dtls-test.pcap") else null dtlsClient.remoteFingerprints = mapOf( - dtlsServer.localFingerprintHashFunction to dtlsServer.localFingerprint + dtlsServer.localFingerprintHashFunction to listOf(dtlsServer.localFingerprint) ) dtlsServer.remoteFingerprints = mapOf( - dtlsClient.localFingerprintHashFunction to dtlsClient.localFingerprint + dtlsClient.localFingerprintHashFunction to listOf(dtlsClient.localFingerprint) ) // The DTLS server's send is wired directly to the DTLS client's receive diff --git a/jvb/src/main/kotlin/org/jitsi/videobridge/Endpoint.kt b/jvb/src/main/kotlin/org/jitsi/videobridge/Endpoint.kt index f904078f9b..99146041c5 100644 --- a/jvb/src/main/kotlin/org/jitsi/videobridge/Endpoint.kt +++ b/jvb/src/main/kotlin/org/jitsi/videobridge/Endpoint.kt @@ -788,11 +788,12 @@ class Endpoint @JvmOverloads constructor( * transport information. */ fun setTransportInfo(transportInfo: IceUdpTransportPacketExtension) { - val remoteFingerprints = mutableMapOf() + val remoteFingerprints = mutableMapOf>() val fingerprintExtensions = transportInfo.getChildExtensionsOfType(DtlsFingerprintPacketExtension::class.java) fingerprintExtensions.forEach { fingerprintExtension -> if (fingerprintExtension.hash != null && fingerprintExtension.fingerprint != null) { - remoteFingerprints[fingerprintExtension.hash] = fingerprintExtension.fingerprint + remoteFingerprints.getOrPut(fingerprintExtension.hash.lowercase()) { mutableListOf() } + .add(fingerprintExtension.fingerprint) } else { logger.info("Ignoring empty DtlsFingerprint extension: ${transportInfo.toStringOpt()}") } diff --git a/jvb/src/main/kotlin/org/jitsi/videobridge/relay/Relay.kt b/jvb/src/main/kotlin/org/jitsi/videobridge/relay/Relay.kt index cbb6e70e1c..6d0beff72e 100644 --- a/jvb/src/main/kotlin/org/jitsi/videobridge/relay/Relay.kt +++ b/jvb/src/main/kotlin/org/jitsi/videobridge/relay/Relay.kt @@ -612,11 +612,12 @@ class Relay @JvmOverloads constructor( * transport information. */ fun setTransportInfo(transportInfo: IceUdpTransportPacketExtension) { - val remoteFingerprints = mutableMapOf() + val remoteFingerprints = mutableMapOf>() val fingerprintExtensions = transportInfo.getChildExtensionsOfType(DtlsFingerprintPacketExtension::class.java) fingerprintExtensions.forEach { fingerprintExtension -> if (fingerprintExtension.hash != null && fingerprintExtension.fingerprint != null) { - remoteFingerprints[fingerprintExtension.hash] = fingerprintExtension.fingerprint + remoteFingerprints.getOrPut(fingerprintExtension.hash.lowercase()) { mutableListOf() } + .add(fingerprintExtension.fingerprint) } else { logger.info("Ignoring empty DtlsFingerprint extension: ${transportInfo.toStringOpt()}") } diff --git a/jvb/src/main/kotlin/org/jitsi/videobridge/transport/dtls/DtlsTransport.kt b/jvb/src/main/kotlin/org/jitsi/videobridge/transport/dtls/DtlsTransport.kt index e6db0a8293..ae59765f08 100644 --- a/jvb/src/main/kotlin/org/jitsi/videobridge/transport/dtls/DtlsTransport.kt +++ b/jvb/src/main/kotlin/org/jitsi/videobridge/transport/dtls/DtlsTransport.kt @@ -163,7 +163,7 @@ class DtlsTransport(parentLogger: Logger, id: String) { } } - fun setRemoteFingerprints(remoteFingerprints: Map) { + fun setRemoteFingerprints(remoteFingerprints: Map>) { // Don't pass an empty list to the stack in order to avoid wiping // certificates that were contained in a previous request. if (remoteFingerprints.isEmpty()) {