Skip to content
Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sergio-correia committed Dec 14, 2021
1 parent 1d1874b commit e82459f
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 25 deletions.
31 changes: 16 additions & 15 deletions src/keys.c
Expand Up @@ -267,20 +267,7 @@ find_by_thp(struct tang_keys_info* tki, const char* target)
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;
Expand Down Expand Up @@ -445,7 +432,21 @@ find_jws(struct tang_keys_info* tki, const char* thp)
}
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*
Expand Down
16 changes: 8 additions & 8 deletions tests/adv
Expand Up @@ -40,11 +40,11 @@ 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
Expand All @@ -56,17 +56,17 @@ 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
Expand Down
5 changes: 5 additions & 0 deletions tests/helpers
Expand Up @@ -84,3 +84,8 @@ valid_key_perm() {
fi
[ "${_perm}" = "440" ]
}

expected_fail () {
echo "Test was expected to fail" >&2
exit 1
}
4 changes: 2 additions & 2 deletions tests/rec
Expand Up @@ -45,8 +45,8 @@ 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`
Expand Down
5 changes: 5 additions & 0 deletions tests/test-keys.c.in
Expand Up @@ -158,6 +158,11 @@ test_find_jws(void)
{"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},
Expand Down

0 comments on commit e82459f

Please sign in to comment.