Mailing List Archive

find-principals was verifying key validity when using ca certs but not
with simple key lifetimes within the allowed signers file.
Since it returns the first keys principal it finds this could result in
a principal with an expired key even though a valid one is just below.

- Add return of matching principals functionality to
check_allowed_keys_line(). This way we can reuse this function and avoid
behaviour differences between find-principals & verify calls.
Use this function for find-principals then as well fixing the above
mentioned bug.
- Add test cases for it.
- Add test for find-principals & verify when multiple identical keys are
present just having different key lifetimes. This could for example happen
when a committer looses access to a repo for a time and regains it later.
This way we can only mark signatures in the respective timespans valid.
- Remove the now unused second check function

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
regress/sshsig.sh | 103 ++++++++++++++++++++++++++++++++++++++++++++++
sshsig.c | 97 +++++++++++++++----------------------------
2 files changed, 136 insertions(+), 64 deletions(-)

diff --git a/regress/sshsig.sh b/regress/sshsig.sh
index 6c96cd5a..788e63ff 100644
--- a/regress/sshsig.sh
+++ b/regress/sshsig.sh
@@ -149,6 +149,26 @@ for t in $SIGNKEYS; do
< $DATA >/dev/null 2>&1 && \
fail "failed signature for $t with expired key"

+ # key lifespan valid
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="19850101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 || \
+ fail "failed find-principals for $t key with valid expiry interval"
+ # key not yet valid
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="19790101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 && \
+ fail "failed find-principals for $t not-yet-valid key"
+ # key expired
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="19990101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 && \
+ fail "failed find-principals for $t with expired key"
+ # NB. assumes we're not running this test in the 1980s
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 && \
+ fail "failed find-principals for $t with expired key"
+
# public key in revoked keys file
cat $pubkey > $OBJ/revoked_keys
(printf "$sig_principal namespaces=\"whatever\" " ;
@@ -205,12 +225,89 @@ for t in $SIGNKEYS; do
# Move private key back
mv ${privkey}.tmp ${privkey}

+ # Duplicate principals & keys in allowed_signers but with different validities
+ ( printf "$sig_principal " ;
+ printf "valid-after=\"19800101\",valid-before=\"19900101\" " ;
+ cat $pubkey;
+ printf "${sig_principal} " ;
+ printf "valid-after=\"19850101\",valid-before=\"20000101\" " ;
+ cat $pubkey) > $OBJ/allowed_signers
+
+ # find-principals outside of any validity lifespan
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="20100101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 && \
+ fail "succeeded find-principals for $t verify-time outside of validity"
+ # find-principals matching only the first lifespan
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="19830101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 || \
+ fail "failed find-principals for $t verify-time within first span"
+ # find-principals matching both lifespans
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="19880101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 || \
+ fail "failed find-principals for $t verify-time within both spans"
+ # find-principals matching only the second lifespan
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="19950101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 || \
+ fail "failed find-principals for $t verify-time within second span"
+
+ # verify outside of any validity lifespan
+ ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \
+ -Overify-time="20100101" -I $sig_principal \
+ -r $OBJ/revoked_keys -f $OBJ/allowed_signers \
+ < $DATA >/dev/null 2>&1 && \
+ fail "succeeded verify for $t verify-time outside of validity"
+ # verify matching only the first lifespan
+ ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \
+ -Overify-time="19830101" -I $sig_principal \
+ -r $OBJ/revoked_keys -f $OBJ/allowed_signers \
+ < $DATA >/dev/null 2>&1 || \
+ fail "failed verify for $t verify-time within first span"
+ # verify matching both lifespans
+ ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \
+ -Overify-time="19880101" -I $sig_principal \
+ -r $OBJ/revoked_keys -f $OBJ/allowed_signers \
+ < $DATA >/dev/null 2>&1 || \
+ fail "failed verify for $t verify-time within both spans"
+ # verify matching only the second lifespan
+ ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \
+ -Overify-time="19950101" -I $sig_principal \
+ -r $OBJ/revoked_keys -f $OBJ/allowed_signers \
+ < $DATA >/dev/null 2>&1 || \
+ fail "failed verify for $t verify-time within second span"
+
# Remaining tests are for certificates only.
case "$keybase" in
*-cert) ;;
*) continue ;;
esac

+ # Check key lifespan on find-principals when using the CA
+ ( printf "$sig_principal " ;
+ printf "cert-authority,valid-after=\"19800101\",valid-before=\"19900101\" ";
+ cat $CA_PUB) > $OBJ/allowed_signers
+ # key lifespan valid
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="19850101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 || \
+ fail "failed find-principals for $t key with valid expiry interval"
+ # key not yet valid
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="19790101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 && \
+ fail "failed find-principals for $t not-yet-valid key"
+ # key expired
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time="19990101" \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 && \
+ fail "failed find-principals for $t with expired key"
+ # NB. assumes we're not running this test in the 1980s
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 && \
+ fail "failed find-principals for $t with expired key"

# correct CA key
(printf "$sig_principal cert-authority " ;
@@ -221,6 +318,12 @@ for t in $SIGNKEYS; do
< $DATA >/dev/null 2>&1 || \
fail "failed signature for $t cert"

+ # find-principals
+ ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+ -Overify-time=19850101 \
+ -f $OBJ/allowed_signers >/dev/null 2>&1 || \
+ fail "failed find-principals for $t with ca key"
+
# signing key listed as cert-authority
(printf "$sig_principal cert-authority " ;
cat $pubkey) > $OBJ/allowed_signers
diff --git a/sshsig.c b/sshsig.c
index 40d132d3..5f8a7b16 100644
--- a/sshsig.c
+++ b/sshsig.c
@@ -869,17 +869,21 @@ cert_filter_principals(const char *path, u_long linenum,
static int
check_allowed_keys_line(const char *path, u_long linenum, char *line,
const struct sshkey *sign_key, const char *principal,
- const char *sig_namespace, uint64_t verify_time)
+ const char *sig_namespace, uint64_t verify_time, char **principalsp)
{
struct sshkey *found_key = NULL;
+ char *principals = NULL;
int r, success = 0;
const char *reason = NULL;
struct sshsigopt *sigopts = NULL;
char tvalid[64], tverify[64];

+ if (principalsp != NULL)
+ *principalsp = NULL;
+
/* Parse the line */
if ((r = parse_principals_key_and_options(path, linenum, line,
- principal, NULL, &found_key, &sigopts)) != 0) {
+ principal, &principals, &found_key, &sigopts)) != 0) {
/* error already logged */
goto done;
}
@@ -889,14 +893,26 @@ check_allowed_keys_line(const char *path, u_long linenum, char *line,
debug("%s:%lu: matched key", path, linenum);
} else if (sigopts->ca && sshkey_is_cert(sign_key) &&
sshkey_equal_public(sign_key->cert->signature_key, found_key)) {
- /* Match of certificate's CA key */
- if ((r = sshkey_cert_check_authority(sign_key, 0, 1, 0,
- verify_time, principal, &reason)) != 0) {
- error("%s:%lu: certificate not authorized: %s",
- path, linenum, reason);
- goto done;
+ if (principal) {
+ /* Match of certificate's CA key with a specified principal */
+ if ((r = sshkey_cert_check_authority(sign_key, 0, 1, 0,
+ verify_time, principal, &reason)) != 0) {
+ error("%s:%lu: certificate not authorized: %s",
+ path, linenum, reason);
+ goto done;
+ }
+ debug("%s:%lu: matched certificate CA key", path, linenum);
+ } else {
+ /* No principal specified - find all matching ones */
+ if ((r = cert_filter_principals(path, linenum,
+ &principals, sign_key, verify_time)) != 0) {
+ /* error already displayed */
+ debug_r(r, "%s:%lu: cert_filter_principals",
+ path, linenum);
+ goto done;
+ }
+ debug("%s:%lu: matched certificate CA key", path, linenum);
}
- debug("%s:%lu: matched certificate CA key", path, linenum);
} else {
/* Didn't match key */
goto done;
@@ -933,6 +949,11 @@ check_allowed_keys_line(const char *path, u_long linenum, char *line,
success = 1;

done:
+ if (success && principalsp != NULL) {
+ *principalsp = principals;
+ principals = NULL; /* transferred */
+ }
+ free(principals);
sshkey_free(found_key);
sshsigopt_free(sigopts);
return success ? 0 : SSH_ERR_KEY_NOT_FOUND;
@@ -960,7 +981,7 @@ sshsig_check_allowed_keys(const char *path, const struct sshkey *sign_key,
while (getline(&line, &linesize, f) != -1) {
linenum++;
r = check_allowed_keys_line(path, linenum, line, sign_key,
- principal, sig_namespace, verify_time);
+ principal, sig_namespace, verify_time, NULL);
free(line);
line = NULL;
linesize = 0;
@@ -979,58 +1000,6 @@ sshsig_check_allowed_keys(const char *path, const struct sshkey *sign_key,
return r == 0 ? SSH_ERR_KEY_NOT_FOUND : r;
}

-static int
-get_matching_principals_from_line(const char *path, u_long linenum, char *line,
- const struct sshkey *sign_key, uint64_t verify_time, char **principalsp)
-{
- struct sshkey *found_key = NULL;
- char *principals = NULL;
- int r, found = 0;
- struct sshsigopt *sigopts = NULL;
-
- if (principalsp != NULL)
- *principalsp = NULL;
-
- /* Parse the line */
- if ((r = parse_principals_key_and_options(path, linenum, line,
- NULL, &principals, &found_key, &sigopts)) != 0) {
- /* error already logged */
- goto done;
- }
-
- if (!sigopts->ca && sshkey_equal(found_key, sign_key)) {
- /* Exact match of key */
- debug("%s:%lu: matched key", path, linenum);
- /* success */
- found = 1;
- } else if (sigopts->ca && sshkey_is_cert(sign_key) &&
- sshkey_equal_public(sign_key->cert->signature_key, found_key)) {
- /* Remove principals listed in file but not allowed by cert */
- if ((r = cert_filter_principals(path, linenum,
- &principals, sign_key, verify_time)) != 0) {
- /* error already displayed */
- debug_r(r, "%s:%lu: cert_filter_principals",
- path, linenum);
- goto done;
- }
- debug("%s:%lu: matched certificate CA key", path, linenum);
- /* success */
- found = 1;
- } else {
- /* Key didn't match */
- goto done;
- }
- done:
- if (found && principalsp != NULL) {
- *principalsp = principals;
- principals = NULL; /* transferred */
- }
- free(principals);
- sshkey_free(found_key);
- sshsigopt_free(sigopts);
- return found ? 0 : SSH_ERR_KEY_NOT_FOUND;
-}
-
int
sshsig_find_principals(const char *path, const struct sshkey *sign_key,
uint64_t verify_time, char **principals)
@@ -1051,8 +1020,8 @@ sshsig_find_principals(const char *path, const struct sshkey *sign_key,

while (getline(&line, &linesize, f) != -1) {
linenum++;
- r = get_matching_principals_from_line(path, linenum, line,
- sign_key, verify_time, principals);
+ r = check_allowed_keys_line(path, linenum, line,
+ sign_key, NULL, NULL, verify_time, principals);
free(line);
line = NULL;
linesize = 0;
--
2.31.1

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev