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 <ijackson@chiark.greenend.org.uk>

diff --git a/NOTES b/NOTES
index 33c010e47d18100f2edc5b53bd8d63afbfdd4f13..ddd14a59c31a29143a0d6fc2acaa12e0c427ff23 100644 (file)
--- a/NOTES
+++ b/NOTES
@@ -174,8 +174,9 @@ quite stable so the feature doesn't gain us much.
 
 Definitions:
 
 
 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
 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
 
 
 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...)
 
 
 (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.
 
 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.
 
 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.
 
 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 8fa4c113ffb33be343c3359e7a1254c29b44e708..2e8335ae634bb297035f2761f0ec6a4ecb06810e 100644 (file)
--- 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)
 
     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;
 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;
     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;
 }
 
     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)
 {
 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,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) {
     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;
 }
 
     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)
 {
 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. */ 
 
     /* 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;
     }
        *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;
     }
        *error="wrong local site name";
        return False;
     }