BER decoding: Improve error checking for indefinite length

When an indefinite length was given, the decoder could look beyond the end of the buffer for the 0,0 that signals the end of the value.
erlang · Aug 11, 2014 · 7f385eb · 7f385eb
1 parent 2390a7c
commit 7f385eb
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 22 deletions.
diff --git a/lib/asn1/c_src/asn1_erl_nif.c b/lib/asn1/c_src/asn1_erl_nif.c
@@ -941,16 +941,31 @@ static int ber_decode_value(ErlNifEnv* env, ERL_NIF_TERM *value, unsigned char *
     int maybe_ret;
     unsigned int len = 0;
     unsigned int lenoflen = 0;
-    int indef = 0;
     unsigned char *tmp_out_buff;
     ERL_NIF_TERM term = 0, curr_head = 0;
 
     if (((in_buf[*ib_index]) & 0x80) == ASN1_SHORT_DEFINITE_LENGTH) {
 	len = in_buf[*ib_index];
-    } else if (in_buf[*ib_index] == ASN1_INDEFINITE_LENGTH
-    )
-	indef = 1;
-    else /* long definite length */{
+    } else if (in_buf[*ib_index] == ASN1_INDEFINITE_LENGTH) {
+	(*ib_index)++;
+	curr_head = enif_make_list(env, 0);
+	if (*ib_index+1 >= in_buf_len) {
+	    return ASN1_INDEF_LEN_ERROR;
+	}
+	while (!(in_buf[*ib_index] == 0 && in_buf[*ib_index + 1] == 0)) {
+	    maybe_ret = ber_decode(env, &term, in_buf, ib_index, in_buf_len);
+	    if (maybe_ret <= ASN1_ERROR) {
+		return maybe_ret;
+	    }
+	    curr_head = enif_make_list_cell(env, term, curr_head);
+	    if (*ib_index+1 >= in_buf_len) {
+		return ASN1_INDEF_LEN_ERROR;
+	    }
+	}
+	enif_make_reverse_list(env, curr_head, value);
+	(*ib_index) += 2; /* skip the indefinite length end bytes */
+	return ASN1_OK;
+    } else /* long definite length */{
 	lenoflen = (in_buf[*ib_index] & 0x7f); /*length of length */
 	if (lenoflen > (in_buf_len - (*ib_index + 1)))
 	    return ASN1_LEN_ERROR;
@@ -965,23 +980,7 @@ static int ber_decode_value(ErlNifEnv* env, ERL_NIF_TERM *value, unsigned char *
     if (len > (in_buf_len - (*ib_index + 1)))
 	return ASN1_VALUE_ERROR;
     (*ib_index)++;
-    if (indef == 1) { /* in this case it is desireably to check that indefinite length
-     end bytes exist in inbuffer */
-	curr_head = enif_make_list(env, 0);
-	while (!(in_buf[*ib_index] == 0 && in_buf[*ib_index + 1] == 0)) {
-	    if (*ib_index >= in_buf_len)
-		return ASN1_INDEF_LEN_ERROR;
-
-	    if ((maybe_ret = ber_decode(env, &term, in_buf, ib_index, in_buf_len))
-		    <= ASN1_ERROR
-		    )
-		return maybe_ret;
-	    curr_head = enif_make_list_cell(env, term, curr_head);
-	}
-	enif_make_reverse_list(env, curr_head, value);
-	(*ib_index) += 2; /* skip the indefinite length end bytes */
-    } else if (form == ASN1_CONSTRUCTED)
-    {
+    if (form == ASN1_CONSTRUCTED) {
 	int end_index = *ib_index + len;
 	if (end_index > in_buf_len)
 	    return ASN1_LEN_ERROR;

diff --git a/lib/asn1/test/ber_decode_error.erl b/lib/asn1/test/ber_decode_error.erl
@@ -51,4 +51,18 @@ run([]) ->
     {error,{asn1,{invalid_value,_}}} =
 	(catch 'Constructed':decode('I', <<8,7>>)),
 
+    %% Short indefinite length. Make sure that the decoder doesn't look
+    %% beyond the end of binary when looking for a 0,0 terminator.
+    {error,{asn1,{invalid_length,_}}} =
+	(catch 'Constructed':decode('S', sub(<<8,16#80,0,0>>, 3))),
+    {error,{asn1,{invalid_length,_}}} =
+	(catch 'Constructed':decode('S', sub(<<8,16#80,0,0>>, 2))),
+    {error,{asn1,{invalid_length,_}}} =
+	(catch 'Constructed':decode('S', sub(<<40,16#80,1,1,255,0,0>>, 6))),
+    {error,{asn1,{invalid_length,_}}} =
+	(catch 'Constructed':decode('S', sub(<<40,16#80,1,1,255,0,0>>, 5))),
     ok.
+
+sub(Bin, Bytes) ->
+    <<B:Bytes/binary,_/binary>> = Bin,
+    B.