CVE-2021-4076: Insufficient Information (NVD-CWE-noinfo)
value: A flaw exists in tang, a network-based cryptographic binding server, which could result in leak of private keys.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
CVSS v3 base score: 7.5
Published: 2022-03-02T23:15Z
Repository: https://github.com/latchset/tang
Language: Other
Files changed: src/keys.c, tests/adv, tests/helpers, tests/rec, tests/test-keys.c.in
Commit date: 2021-12-14 12:18:41 +0100
Commit message: keys: move signing part out of find_by_thp() and to find_jws() (#81)

Handle just signing keys in find_jws(), to make sure we are
responding only to proper queries.

Tests were also failing to detect this issue and were updated
accordingly.

Issue discovered by Twitter Kernel and OS team during a source
code audit while evaluating Tang/Clevis for their needs.

Fixes CVE-2021-4076
Diff: diff --git a/src/keys.c b/src/keys.c
index a433f9c..205bbe5 100644
--- a/src/keys.c
+++ b/src/keys.c
@@ -265,24 +265,11 @@ find_by_thp(struct tang_keys_info* tki, const char* target)
         for (int i = 0; hashes[i]; i++) {
             __attribute__ ((__cleanup__(cleanup_str))) char* thumbprint = jwk_thumbprint(jwk, hashes[i]);
             if (!thumbprint || strcmp(thumbprint, target) != 0) {
                 continue;
             }
-
-            if (jwk_valid_for_deriving_keys(jwk)) {
-                return json_incref(jwk);
-            } else if (jwk_valid_for_signing(jwk)) {
-                json_auto_t* sign = json_deep_copy(tki->m_sign);
-                if (json_array_append(sign, jwk) == -1) {
-                    return NULL;
-                }
-                json_auto_t* jws = jwk_sign(tki->m_payload, sign);
-                if (!jws) {
-                    return NULL;
-                }
-                return json_incref(jws);
-            }
+            return json_incref(jwk);
         }
     }
     return NULL;
 }
 
@@ -443,11 +430,25 @@ find_jws(struct tang_keys_info* tki, const char* thp)
         if (!jws) {
             return NULL;
         }
         return json_incref(jws);
     }
-    return find_by_thp(tki, thp);
+
+    json_auto_t* jwk = find_by_thp(tki, thp);
+    if (!jwk_valid_for_signing(jwk)) {
+        return NULL;
+    }
+
+    json_auto_t* sign = json_deep_copy(tki->m_sign);
+    if (json_array_append(sign, jwk) == -1) {
+        return NULL;
+    }
+    json_auto_t* jws = jwk_sign(tki->m_payload, sign);
+    if (!jws) {
+        return NULL;
+    }
+    return json_incref(jws);
 }
 
 json_t*
 find_jwk(struct tang_keys_info* tki, const char* thp)
 {
diff --git a/tests/adv b/tests/adv
index 1fde37e..3cc1a57 100755
--- a/tests/adv
+++ b/tests/adv
@@ -38,15 +38,15 @@ export PORT=$(random_port)
 start_server "${PORT}"
 export PID=$!
 sleep 0.5
 
 # Make sure requests on the root fail
-! fetch /
+fetch / && expected_fail
 
 # The request should fail (404) for non-signature key IDs
-! fetch /adv/`jose jwk thp -i $TMP/db/exc.jwk`
-! fetch /adv/`jose jwk thp -a S512 -i $TMP/db/exc.jwk`
+fetch /adv/`jose jwk thp -i $TMP/db/exc.jwk` && expected_fail
+fetch /adv/`jose jwk thp -a S512 -i $TMP/db/exc.jwk` && expected_fail
 
 # The default advertisement fetch should succeed and pass verification
 fetch /adv
 fetch /adv | ver $TMP/db/sig.jwk
 fetch /adv/ | ver $TMP/db/sig.jwk
@@ -54,21 +54,21 @@ fetch /adv/ | ver $TMP/db/sig.jwk
 # Fetching by any thumbprint should work
 fetch /adv/`jose jwk thp -i $TMP/db/sig.jwk` | ver $TMP/db/sig.jwk
 fetch /adv/`jose jwk thp -a S512 -i $TMP/db/sig.jwk` | ver $TMP/db/sig.jwk
 
 # Requesting an adv by an advertised key ID should't be signed by hidden keys
-! fetch /adv/`jose jwk thp -i $TMP/db/sig.jwk` | ver $TMP/db/.sig.jwk
-! fetch /adv/`jose jwk thp -i $TMP/db/sig.jwk` | ver $TMP/db/.oth.jwk
+fetch /adv/`jose jwk thp -i $TMP/db/sig.jwk` | ver $TMP/db/.sig.jwk && expected_fail
+fetch /adv/`jose jwk thp -i $TMP/db/sig.jwk` | ver $TMP/db/.oth.jwk && expected_fail
 
 # Verify that the default advertisement is not signed with hidden signature keys
-! fetch /adv/ | ver $TMP/db/.oth.jwk
-! fetch /adv/ | ver $TMP/db/.sig.jwk
+fetch /adv/ | ver $TMP/db/.oth.jwk && expected_fail
+fetch /adv/ | ver $TMP/db/.sig.jwk && expected_fail
 
 # A private key advertisement is signed by all advertised keys and the requested private key
 fetch /adv/`jose jwk thp -i $TMP/db/.sig.jwk` | ver $TMP/db/sig.jwk
 fetch /adv/`jose jwk thp -i $TMP/db/.sig.jwk` | ver $TMP/db/.sig.jwk
-! fetch /adv/`jose jwk thp -i $TMP/db/.sig.jwk` | ver $TMP/db/.oth.jwk
+fetch /adv/`jose jwk thp -i $TMP/db/.sig.jwk` | ver $TMP/db/.oth.jwk && expected_fail
 
 # Verify that the advertisements contain the cty parameter
 fetch /adv | jose fmt -j- -Og protected -SyOg cty -Sq "jwk-set+json" -E
 fetch /adv/`jose jwk thp -i $TMP/db/.sig.jwk` \
     | jose fmt -j- -Og signatures -A \
diff --git a/tests/helpers b/tests/helpers
index 2968b6d..541af08 100755
--- a/tests/helpers
+++ b/tests/helpers
@@ -82,5 +82,10 @@ valid_key_perm() {
     else
         _perm="$(stat -c %a "${1}")"
     fi
     [ "${_perm}" = "440" ]
 }
+
+expected_fail () {
+    echo "Test was expected to fail" >&2
+    exit 1
+}
diff --git a/tests/rec b/tests/rec
index 7fba6a9..736acee 100755
--- a/tests/rec
+++ b/tests/rec
@@ -43,12 +43,12 @@ export PORT=$(random_port)
 start_server "${PORT}"
 export PID=$!
 sleep 0.5
 
 # Make sure that GET fails
-! curl -sf http://127.0.0.1:$PORT/rec
-! curl -sf http://127.0.0.1:$PORT/rec/
+curl -sf http://127.0.0.1:$PORT/rec && expected_fail
+curl -sf http://127.0.0.1:$PORT/rec/ && expected_fail
 
 # Make a recovery request (NOTE: this is insecure! Don't do this in real code!)
 good=`jose jwk exc -i '{"alg":"ECMR","key_ops":["deriveKey"]}' -l $TMP/exc.jwk -r $TMP/db/exc.jwk`
 test=`curl -sf -X POST \
            -H "Content-Type: application/jwk+json" \
diff --git a/tests/test-keys.c.in b/tests/test-keys.c.in
index fca26c4..59556b5 100644
--- a/tests/test-keys.c.in
+++ b/tests/test-keys.c.in
@@ -156,10 +156,15 @@ test_find_jws(void)
         {"qgmqJSo6AEEuVQY7zVlklqdTMqY", 1},
         {"r4E2wG1u_YyKUo0N0rIK7jJF5Xg", 1},
         {"ugJ4Ula-YABQIiJ-0g3B_jpFpF2nl3W-DNpfLdXArhTusV0QCcd1vtgDeGHEPzpm7jEsyC7VYYSSOkZicK22mw", 1},
         {"up0Z4fRhpd4O5QwBaMCXDTlrvxCmZacU0MD8kw", 1},
         {"vllHS-M0aQFCo2yUCcAahMU4TAtXACyeuRf-zbmmTPBg7V0Pb-RRFGo5C6MnpzdirK8B3ORLOsN8RyXClvtjxA", 1},
+        {"-bWkGaJi0Zdvxaj4DCp28umLcRA", 0},
+        {"WEpfFyeoNKkE2-TosN_bP-gd9UgRvQCZpVasZQ", 0},
+        {"L4xg2tZXTEVbsK39bzOZM1jGWn3HtOxF5gh6F9YVf5Q", 0},
+        {"9U8qgy_YjyY6Isuq6QuiKEiYZgNJShcGgJx5FJzCu6m3N6zFaIPy_HDkxkVqAZ9E", 0},
+        {"Cy73glFjs6B6RU7wy6vWxAc-2bJy5VJOT9LyK80eKgZ8k27wXZ-3rjsuNU5tua_yHWtluyoSYtjoKXfI0E8ESw", 0},
         {NULL, 1},
         {"a", 0},
         {"foo", 0},
         {"bar", 0},
         {"XXXXXXXXXXXXXXXXXX", 0}

Vulnerable code: 
            if (jwk_valid_for_deriving_keys(jwk)) {
                return json_incref(jwk);
            } else if (jwk_valid_for_signing(jwk)) {
                json_auto_t* sign = json_deep_copy(tki->m_sign);
                if (json_array_append(sign, jwk) == -1) {
                    return NULL;
                }
                json_auto_t* jws = jwk_sign(tki->m_payload, sign);
                if (!jws) {
                    return NULL;
                }
                return json_incref(jws);
            }
    return find_by_thp(tki, thp);
! fetch /
! fetch /adv/`jose jwk thp -i $TMP/db/exc.jwk`
! fetch /adv/`jose jwk thp -a S512 -i $TMP/db/exc.jwk`
! fetch /adv/`jose jwk thp -i $TMP/db/sig.jwk` | ver $TMP/db/.sig.jwk
! fetch /adv/`jose jwk thp -i $TMP/db/sig.jwk` | ver $TMP/db/.oth.jwk
! fetch /adv/ | ver $TMP/db/.oth.jwk
! fetch /adv/ | ver $TMP/db/.sig.jwk
! fetch /adv/`jose jwk thp -i $TMP/db/.sig.jwk` | ver $TMP/db/.oth.jwk
! curl -sf http://127.0.0.1:$PORT/rec
! curl -sf http://127.0.0.1:$PORT/rec/
Fixed code:             return json_incref(jwk);

    json_auto_t* jwk = find_by_thp(tki, thp);
    if (!jwk_valid_for_signing(jwk)) {
        return NULL;
    }

    json_auto_t* sign = json_deep_copy(tki->m_sign);
    if (json_array_append(sign, jwk) == -1) {
        return NULL;
    }
    json_auto_t* jws = jwk_sign(tki->m_payload, sign);
    if (!jws) {
        return NULL;
    }
    return json_incref(jws);
fetch / && expected_fail
fetch /adv/`jose jwk thp -i $TMP/db/exc.jwk` && expected_fail
fetch /adv/`jose jwk thp -a S512 -i $TMP/db/exc.jwk` && expected_fail
fetch /adv/`jose jwk thp -i $TMP/db/sig.jwk` | ver $TMP/db/.sig.jwk && expected_fail
fetch /adv/`jose jwk thp -i $TMP/db/sig.jwk` | ver $TMP/db/.oth.jwk && expected_fail
fetch /adv/ | ver $TMP/db/.oth.jwk && expected_fail
fetch /adv/ | ver $TMP/db/.sig.jwk && expected_fail
fetch /adv/`jose jwk thp -i $TMP/db/.sig.jwk` | ver $TMP/db/.oth.jwk && expected_fail

expected_fail () {
    echo "Test was expected to fail" >&2
    exit 1
}
curl -sf http://127.0.0.1:$PORT/rec && expected_fail
curl -sf http://127.0.0.1:$PORT/rec/ && expected_fail
        {"-bWkGaJi0Zdvxaj4DCp28umLcRA", 0},
        {"WEpfFyeoNKkE2-TosN_bP-gd9UgRvQCZpVasZQ", 0},
        {"L4xg2tZXTEVbsK39bzOZM1jGWn3HtOxF5gh6F9YVf5Q", 0},
        {"9U8qgy_YjyY6Isuq6QuiKEiYZgNJShcGgJx5FJzCu6m3N6zFaIPy_HDkxkVqAZ9E", 0},
        {"Cy73glFjs6B6RU7wy6vWxAc-2bJy5VJOT9LyK80eKgZ8k27wXZ-3rjsuNU5tua_yHWtluyoSYtjoKXfI0E8ESw", 0},