From 716d3f57b471b2e2c3b5772008f5d32767c6cbeb Mon Sep 17 00:00:00 2001 From: Patrik Nyblom Date: Wed, 25 Aug 2010 11:47:11 +0200 Subject: Remove two buffer overflow vulnerabilities in EPMD --- erts/epmd/src/epmd_int.h | 1 + erts/epmd/src/epmd_srv.c | 28 ++++++++++++------- erts/epmd/test/epmd_SUITE.erl | 62 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 79 insertions(+), 12 deletions(-) (limited to 'erts') diff --git a/erts/epmd/src/epmd_int.h b/erts/epmd/src/epmd_int.h index 5ead553f36..d69d8c93ef 100644 --- a/erts/epmd/src/epmd_int.h +++ b/erts/epmd/src/epmd_int.h @@ -1,3 +1,4 @@ +/* -*- c-indent-level: 2; c-continued-statement-offset: 2 -*- */ /* * %CopyrightBegin% * diff --git a/erts/epmd/src/epmd_srv.c b/erts/epmd/src/epmd_srv.c index 12ebd7f415..9e470e41b0 100644 --- a/erts/epmd/src/epmd_srv.c +++ b/erts/epmd/src/epmd_srv.c @@ -270,11 +270,9 @@ static void do_read(EpmdVars *g,Connection *s) s->fd,val); dbg_print_buf(g,s->buf,val); - /* FIXME: Shouldn't be needed to close down.... */ node_unreg_sock(g,s->fd); epmd_conn_close(g,s); } - /* FIXME: We always close, probably the right thing to do */ return; } @@ -372,6 +370,8 @@ static int do_accept(EpmdVars *g,int listensock) return conn_open(g,msgsock); } +/* buf is actually one byte larger than bsize, + giving place for null termination */ static void do_request(g, fd, s, buf, bsize) EpmdVars *g; int fd; @@ -382,13 +382,6 @@ static void do_request(g, fd, s, buf, bsize) char wbuf[OUTBUF_SIZE]; /* Buffer for writing */ int i; - /* - * Terminate packet as a C string. Needed for requests received from Erlang - * nodes with lower version than R3A. - */ - - buf[bsize] = '\0'; - switch (*buf) { case EPMD_ALIVE2_REQ: @@ -398,7 +391,7 @@ static void do_request(g, fd, s, buf, bsize) in network byte order, and yyyyyy is symname, possibly null terminated. */ - if (bsize <= 13) + if (bsize <= 14) /* at least one character for the node name */ { dbg_printf(g,0,"packet to small for request ALIVE2_REQ (%d)",bsize); return; @@ -421,7 +414,17 @@ static void do_request(g, fd, s, buf, bsize) highvsn = get_int16(&buf[5]); lowvsn = get_int16(&buf[7]); namelen = get_int16(&buf[9]); + if (namelen + 13 > bsize) { + dbg_printf(g,0,"Node name size error in ALIVE2_REQ"); + return; + } extralen = get_int16(&buf[11+namelen]); + + if (extralen + namelen + 13 > bsize) { + dbg_printf(g,0,"Extra info size error in ALIVE2_REQ"); + return; + } + for (i = 11 ; i < 11 + namelen; i ++) if (buf[i] == '\000') { dbg_printf(g,0,"node name contains ascii 0 in ALIVE2_REQ"); @@ -888,6 +891,11 @@ static Node *node_reg2(EpmdVars *g, dbg_printf(g,0,"node name is too long (%d) %s", strlen(name), name); return NULL; } + if (extralen > MAXSYMLEN) + { + dbg_printf(g,0,"extra data is too long (%d) %s", strlen(name), name); + return NULL; + } /* Fail if it is already registered */ diff --git a/erts/epmd/test/epmd_SUITE.erl b/erts/epmd/test/epmd_SUITE.erl index 88980fec63..5bcbe96b31 100644 --- a/erts/epmd/test/epmd_SUITE.erl +++ b/erts/epmd/test/epmd_SUITE.erl @@ -63,7 +63,9 @@ alive_req_too_large/1, returns_valid_empty_extra/1, - returns_valid_populated_extra_with_nulls/1 + returns_valid_populated_extra_with_nulls/1, + buffer_overrun_1/1, + buffer_overrun_2/1 ]). @@ -121,7 +123,10 @@ all(suite) -> alive_req_too_large, returns_valid_empty_extra, - returns_valid_populated_extra_with_nulls + returns_valid_populated_extra_with_nulls, + + buffer_overrun_1, + buffer_overrun_2 ]. %% @@ -705,6 +710,59 @@ returns_valid_populated_extra_with_nulls(Config) when is_list(Config) -> ?line ok = close(Sock), ok. + +buffer_overrun_1(suite) -> + []; +buffer_overrun_1(doc) -> + ["Test security vulnerability in fake extra lengths in alive2_req"]; +buffer_overrun_1(Config) when is_list(Config) -> + ?line ok = epmdrun(), + ?line true = alltrue([hostile(N) || N <- lists:seq(1,10000)]), + ok. +buffer_overrun_2(suite) -> + []; +buffer_overrun_2(doc) -> + ["Test security vulnerability in fake extra lengths in alive2_req"]; +buffer_overrun_2(Config) when is_list(Config) -> + ?line ok = epmdrun(), + ?line [false | Rest] = [hostile2(N) || N <- lists:seq(255,10000)], + ?line true = alltrue(Rest), + ok. +hostile(N) -> + try + Bin= <<$x:8,4747:16,$M:8,0:8,5:16,5:16,5:16,"gurka",N:16>>, + S = size(Bin), + {ok,E}=connect(), + gen_tcp:send(E,[<>,Bin]), + closed = recv(E,1), + gen_tcp:close(E), + true + catch + _:_ -> + false + end. +hostile2(N) -> + try + B2 = list_to_binary(lists:duplicate(N,255)), + Bin= <<$x:8,4747:16,$M:8,0:8,5:16,5:16,5:16,"gurka",N:16,B2/binary>>, + S = size(Bin), + {ok,E}=connect(), + gen_tcp:send(E,[<>,Bin]), + Z = recv(E,2), + gen_tcp:close(E), + (Z =:= closed) or (Z =:= {ok, [$y,1]}) + catch + _:_ -> + false + end. + +alltrue([]) -> + true; +alltrue([true|T]) -> + alltrue(T); +alltrue([_|_]) -> + false. + %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% % Terminate all tests with killing epmd. -- cgit v1.2.3