From 1737eeef9bc4aec1b4d7baa220ce48238b498006 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:51 +0100 Subject: [PATCH] site: fix site name checking leaving room for expansion Previously, secnet would only check that the site names sent by the peer had the expected site names as prefixes. This would be a security bug on a vpn where one site name is the prefix of another site name. In fact, if the peer sends a short name, secnet will suffer a buffer read overrun and maybe crash - although this is exploitable only for DoS at worst, and not as an information leak. (With reasonable peers, it won't cause packets to be mishandled, because the next field after each name string is a 16-byte integer giving a name or public key byte count, which would have to contain a value greater than 8000 to look like printable characters.) Fix this bug. However, we use this as an opportunity to provide some space (in the signed part of MSG2..4) for future extensions. (At present there is nothing in the parts of MSG3 and MSG4 covered by the signature which is not supposedly entirely fixed - apart from the amount of zero-padding at the MS end of our DH public exponent. This is bad because it provides no way to do transport protocol upgrade/downgrade negotiation without leaving open a protocol version downgrade attack.) After this change the name fields in MSG2..4 are, following the 16-bit length, either just the characters of the name, or the name, followed by a nul byte, followed by zero or more bytes of additional data. Additional data beyond what the receiver expects (which currently means any additional data), is to be ignored. Let us work through some examples. Suppose the expected site name is "expected". Here are the behaviours in relation to various names being transmitted in the packet. (This applies both to the local name, and to the remote name; "rejected" means other instances of the protocol engine get a go, and perhaps the instance with the correct names will accept the message.) MSG Sent name Old secnet New secnet 1 expected ok ok 1 expected\0extra rejected rejected 1 expected/suffix rejected rejected 1 expect rejected rejected 2-4 expected ok ok 2-4 expected\0extra ok, extra ignored ok, extra data 2-4 expected/suffix wrongly accepted rejected 2-4 expect read overrun rejected We will do the same for MSG1 in a later commit. Signed-off-by: Ian Jackson --- NOTES | 11 ++++++----- site.c | 53 +++++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/NOTES b/NOTES index 33c010e..ddd14a5 100644 --- a/NOTES +++ b/NOTES @@ -174,8 +174,9 @@ quite stable so the feature doesn't gain us much. Definitions: -A is the originating gateway machine -B is the destination gateway machine +A is the originating gateway machine name +B is the destination gateway machine name +A+ and B+ are the names with optional additional data, currently ignored PK_A is the public RSA key of A PK_B is the public RSA key of B PK_A^-1 is the private RSA key of A @@ -199,12 +200,12 @@ Messages: 1) A->B: *,iA,msg1,A,B,nA -2) B->A: iA,iB,msg2,B,A,nB,nA +2) B->A: iA,iB,msg2,B+,A+,nB,nA (The order of B and A reverses in alternate messages so that the same code can be used to construct them...) -3) A->B: {iB,iA,msg3,A,B,nA,nB,g^x mod m}_PK_A^-1 +3) A->B: {iB,iA,msg3,A+,B+,nA,nB,g^x mod m}_PK_A^-1 If message 1 was a replay then A will not generate message 3, because it doesn't recognise nA. @@ -212,7 +213,7 @@ it doesn't recognise nA. If message 2 was from an attacker then B will not generate message 4, because it doesn't recognise nB. -4) B->A: {iA,iB,msg4,B,A,nB,nA,g^y mod m}_PK_B^-1 +4) B->A: {iA,iB,msg4,B+,A+,nB,nA,g^y mod m}_PK_B^-1 At this point, A and B share a key, k. B must keep retransmitting message 4 until it receives a packet encrypted using key k. diff --git a/site.c b/site.c index 8fa4c11..2e8335a 100644 --- a/site.c +++ b/site.c @@ -347,14 +347,19 @@ static bool_t current_valid(struct site *st) type=buf_unprepend_uint32((b)); \ if (type!=(t)) return False; } while(0) +struct parsedname { + int32_t len; + uint8_t *name; + int32_t extrainfo_len; + uint8_t *extrainfo; +}; + struct msg { uint8_t *hashstart; uint32_t dest; uint32_t source; - int32_t remlen; - uint8_t *remote; - int32_t loclen; - uint8_t *local; + struct parsedname remote; + struct parsedname local; uint8_t *nR; uint8_t *nL; int32_t pklen; @@ -402,6 +407,24 @@ static bool_t generate_msg(struct site *st, uint32_t type, cstring_t what) return True; } +static bool_t unpick_name(struct buffer_if *msg, struct parsedname *nm) +{ + CHECK_AVAIL(msg,2); + nm->len=buf_unprepend_uint16(msg); + CHECK_AVAIL(msg,nm->len); + nm->name=buf_unprepend(msg,nm->len); + uint8_t *nul=memchr(nm->name,0,nm->len); + if (!nul) { + nm->extrainfo_len=0; + nm->extrainfo=0; + } else { + nm->extrainfo=nul+1; + nm->extrainfo_len=msg->start-nm->extrainfo; + nm->len=nul-nm->name; + } + return True; +} + static bool_t unpick_msg(struct site *st, uint32_t type, struct buffer_if *msg, struct msg *m) { @@ -411,14 +434,8 @@ static bool_t unpick_msg(struct site *st, uint32_t type, CHECK_AVAIL(msg,4); m->source=buf_unprepend_uint32(msg); CHECK_TYPE(msg,type); - CHECK_AVAIL(msg,2); - m->remlen=buf_unprepend_uint16(msg); - CHECK_AVAIL(msg,m->remlen); - m->remote=buf_unprepend(msg,m->remlen); - CHECK_AVAIL(msg,2); - m->loclen=buf_unprepend_uint16(msg); - CHECK_AVAIL(msg,m->loclen); - m->local=buf_unprepend(msg,m->loclen); + if (!unpick_name(msg,&m->remote)) return False; + if (!unpick_name(msg,&m->local)) return False; CHECK_AVAIL(msg,NONCELEN); m->nR=buf_unprepend(msg,NONCELEN); if (type==LABEL_MSG1) { @@ -444,6 +461,14 @@ static bool_t unpick_msg(struct site *st, uint32_t type, return True; } +static bool_t name_matches(const struct parsedname *nm, const char *expected) +{ + int expected_len=strlen(expected); + return + nm->len == expected_len && + !memcmp(nm->name, expected, expected_len); +} + static bool_t check_msg(struct site *st, uint32_t type, struct msg *m, cstring_t *error) { @@ -451,11 +476,11 @@ static bool_t check_msg(struct site *st, uint32_t type, struct msg *m, /* Check that the site names and our nonce have been sent back correctly, and then store our peer's nonce. */ - if (memcmp(m->remote,st->remotename,strlen(st->remotename)!=0)) { + if (!name_matches(&m->remote,st->remotename)) { *error="wrong remote site name"; return False; } - if (memcmp(m->local,st->localname,strlen(st->localname)!=0)) { + if (!name_matches(&m->local,st->localname)) { *error="wrong local site name"; return False; } -- 2.30.2