Repository: EmbeddedRPC/erpc (language: python)

Issue:
[BUG] string length not enforced in client or server
Hi,
First off, trying out eRPC and impressed by what I see so far. Great work.

I have a command with a maximum of string length associated with the parameter. The max length (@max_length) is not properly enforced in either client nor server:
- On client side the generated code simply writes the string with writeString(strlen(), (const char*)str, (const char*)), no validation is done
- On server side it actually looks worse: 

1.  codec->readString() is used, populating size and pointer (uint32_t str_len,  char * str_local)
2. Allocation is done with the @max_length+1
3. If the read string is not NULL and allocation was successful memcpy() is used with the length of the read string (possible buffer overflow if string sent exceeds @max_length)

The issues I see:
1. Client side can and should avoid sending content that violets the @max_length
2. No validation of size, possible buffer overflow(?)

Thank you!


Discussion / hints:
Hi eRPC user. Thank you for your interest and welcome. We hope you will enjoy this framework well.
HI @taldinj, you may be correct that there is missing validation. Thank you for your input. Once i will have time i will improve this behavior.

Fix patch:
diff --git a/erpcgen/src/templates/c_coders.template b/erpcgen/src/templates/c_coders.template
index f0bdc0af..4f7ed1e3 100644
--- a/erpcgen/src/templates/c_coders.template
+++ b/erpcgen/src/templates/c_coders.template
@@ -1,24 +1,40 @@
 {% def decodeBuiltinType(info) --------------- BuiltinType %}
 {% if info.builtinType == "kStringType" %}
-uint32_t {$info.stringLocalName}_len;
-char * {$info.stringLocalName}_local;
-codec->readString(&{$info.stringLocalName}_len, &{$info.stringLocalName}_local);
+{
+    uint32_t {$info.stringLocalName}_len;
+    char * {$info.stringLocalName}_local;
+    codec->readString(&{$info.stringLocalName}_len, &{$info.stringLocalName}_local);
+{%  if info.stringAllocSize != info.stringLocalName & "_len" %}
+    if ({$info.stringLocalName}_len <= {$info.stringAllocSize})
+    {
+{%   set indent = "    " >%}
+{% else %}
+{%   set indent = "" >%}
+{% endif  -- if info.stringAllocSize != info.stringLocalName & "_len" %}
 {%  if ((source == "client" && info.withoutAlloc == false) or source == "server") %}
-{$info.name} = ({$info.builtinTypeName}) erpc_malloc(({$info.stringAllocSize} + 1) * sizeof(char));
+{$indent}    {$info.name} = ({$info.builtinTypeName}) erpc_malloc(({$info.stringAllocSize} + 1) * sizeof(char));
 {%   if generateAllocErrorChecks == true %}
-if (({$info.name} == NULL) || ({$info.stringLocalName}_local == NULL))
-{
-    codec->updateStatus(kErpcStatus_MemoryError);
-}
-else
-{
+{$indent}    if (({$info.name} == NULL) || ({$info.stringLocalName}_local == NULL))
+{$indent}    {
+{$indent}        codec->updateStatus(kErpcStatus_MemoryError);
+{$indent}    }
+{$indent}    else
+{$indent}    {
 {%   endif -- generateAllocErrorChecks == true %}
 {%  endif -- withoutAlloc %}
-{%  if (((source == "client" && info.withoutAlloc == false) or source == "server") && generateAllocErrorChecks == true) %}    {%  endif -- withoutAlloc %}memcpy({$info.name}, {$info.stringLocalName}_local, {$info.stringLocalName}_len);
-{%  if (((source == "client" && info.withoutAlloc == false) or source == "server") && generateAllocErrorChecks == true) %}    {%  endif -- withoutAlloc %}({$info.name})[{$info.stringLocalName}_len] = 0;
+{$indent}    {%  if (((source == "client" && info.withoutAlloc == false) or source == "server") && generateAllocErrorChecks == true) %}    {%  endif -- withoutAlloc %}memcpy({$info.name}, {$info.stringLocalName}_local, {$info.stringLocalName}_len);
+{$indent}    {%  if (((source == "client" && info.withoutAlloc == false) or source == "server") && generateAllocErrorChecks == true) %}    {%  endif -- withoutAlloc %}({$info.name})[{$info.stringLocalName}_len] = 0;
 {%  if (((source == "client" && info.withoutAlloc == false) or source == "server") && generateAllocErrorChecks == true) %}
-}
+    {$indent}}
 {%  endif -- withoutAlloc && generateAllocErrorChecks %}
+{%  if info.stringAllocSize != info.stringLocalName & "_len" %}
+    }
+    else
+    {
+        codec->updateStatus(kErpcStatus_InvalidArgument);
+    }
+{% endif  -- if info.stringAllocSize != info.stringLocalName & "_len" %}
+}
 {% else %}
 {%  if source == "client" && info.pointerScalarTypes %}
 codec->read({$info.name});
@@ -65,7 +81,7 @@ if (({$info.sizeTemp} <= {$info.maxSize}) && ({$info.dataTemp} != NULL))
 }
 else
 {
-    codec->updateStatus(kErpcStatus_Fail);
+    codec->updateStatus(kErpcStatus_InvalidArgument);
 }
 {% endif %}
 {% enddef ------------------------------- BinaryType %}
@@ -228,7 +244,15 @@ codec->readData({$info.name}, {$info.sizeTemp} * sizeof({$info.builtinTypeName})
 {# Encode sending data #}
 {% def encodeBuiltinType(info) -----------------  %}
 {% if info.builtinType == "kStringType" %}
-codec->writeString(strlen((const char*){$info.name}), (const char*){$info.name});
+{
+    uint32_t {$info.stringLocalName}_len = strlen((const char*){$info.name});
+
+{% if info.stringAllocSize != info.stringLocalName & "_len"  %}
+    erpc_assert({$info.stringLocalName}_len <= {$info.stringAllocSize});
+
+{% endif %}
+    codec->writeString({$info.stringLocalName}_len, (const char*){$info.name});
+}
 {% else %}
 {%  if source == "client" && info.pointerScalarTypes %}
 codec->write(*{$info.name});
@@ -239,8 +263,8 @@ codec->write({$info.name});
 {% enddef --------------------------------------- BuiltinType %}
 
 {% def encodeBinaryType(info) -----------------  %}
-{% if source == "server" && info.size != info.maxSize %}
-erpc_assert({$info.size} <= {$info.maxSize} * sizeof({$info.mallocSizeType}));
+{% if info.sizeTemp != info.maxSize %}
+erpc_assert({% if source == "client" && info.pointerScalarTypes %}*{% endif %}{$info.size} <= {$info.maxSize} * sizeof({$info.mallocSizeType}));
 {% endif %}
 codec->writeBinary({% if source == "client" && info.pointerScalarTypes %}*{% endif %}{$info.size}, {$info.name});
 {% enddef --------------------------------------- BinaryType %}


Test patch:
diff --git a/erpcgen/test/test_nullable_c.yml b/erpcgen/test/test_nullable_c.yml
index be20db23..edec1856 100644
--- a/erpcgen/test/test_nullable_c.yml
+++ b/erpcgen/test/test_nullable_c.yml
@@ -16,9 +16,11 @@ test_client.cpp:
   - codec->writeNullFlag(true)
   - if: dir in ('in', 'inout')
     then:
-      - codec->writeString(strlen((const char*)a), (const char*)a);
+      - uint32_t a_len = strlen((const char*)a);
+      - codec->writeString(a_len, (const char*)a);
   - if: dir == 'out'
     then:
+      - not: codec->writeString(a_len, (const char*)a);
       - not: codec->writeString(strlen((const char*)a), (const char*)a);
   - if: dir in ('out', 'inout')
     then:
@@ -41,7 +43,8 @@ test_server.cpp:
   - if: dir in ('out', 'inout')
     then:
       - if (a != NULL)
-      - codec->writeString(strlen((const char*)a), (const char*)a);
+      - uint32_t a_len = strlen((const char*)a);
+      - codec->writeString(a_len, (const char*)a);
 ---
 name: param ustring
 desc:
@@ -60,9 +63,11 @@ test_client.cpp:
   - codec->writeNullFlag(true)
   - if: dir in ('in', 'inout')
     then:
-      - codec->writeString(strlen((const char*)a), (const char*)a);
+      - uint32_t a_len = strlen((const char*)a);
+      - codec->writeString(a_len, (const char*)a);
   - if: dir == 'out'
     then:
+      - not: codec->writeString(a_len, (const char*)a);
       - not: codec->writeString(strlen((const char*)a), (const char*)a);
   - if: dir in ('out', 'inout')
     then:
@@ -85,7 +90,8 @@ test_server.cpp:
   - if: dir in ('out', 'inout')
     then:
       - if (a != NULL)
-      - codec->writeString(strlen((const char*)a), (const char*)a);
+      - uint32_t a_len = strlen((const char*)a);
+      - codec->writeString(a_len, (const char*)a);
 ---
 name: in param struct
 desc:
@@ -212,7 +218,8 @@ test_client.cpp:
       - codec->writeNullFlag(true);
       - else
       - codec->writeNullFlag(false);
-      - codec->writeString(strlen((const char*)data->a), (const char*)data->a);
+      - uint32_t a_len = strlen((const char*)data->a);
+      - codec->writeString(a_len, (const char*)data->a);
   - if: dir in ('out', 'inout')
     then:
       - void read_erpc_pair_struct
@@ -241,7 +248,8 @@ test_server.cpp:
       - codec->writeNullFlag(true);
       - else
       - codec->writeNullFlag(false);
-      - codec->writeString(strlen((const char*)data->a), (const char*)data->a);
+      - uint32_t a_len = strlen((const char*)data->a);
+      - codec->writeString(a_len, (const char*)data->a);
 
 ---
 name: return struct