Fix undefined behavior with bool fields (nanopb#434)

Previously nanopb didn't enforce that decoded bool fields
had valid true/false values. This could lead to undefined
behavior in user code.

This has potential security implications when

1) message contains bool field (has_ fields are safe)
and
2) user code uses ternary operator dependent on the field value,
   such as: int value = msg.my_bool ? 1234 : 0
and
3) the value returned from ternary operator affects a memory access,
   such as: data_array[value] = 9999

docs/migration.rst

@@ -154,6 +154,22 @@ define always has the largest value.
 are not defined in ascending order, user code behaviour may change. Check that
 user code doesn't expect the old, incorrect first/last behaviour.
 
+Fix undefined behavior related to bool fields
+---------------------------------------------
+
+**Rationale:** In C99, `bool` variables are not allowed to have other values
+than `true` and `false`. Compilers use this fact in optimization, and constructs
+like `int foo = msg.has_field ? 100 : 0` will give unexpected results otherwise.
+Previously nanopb didn't enforce that decoded bool fields had valid values.
+
+**Changes:** Bool fields are now handled separately as `PB_LTYPE_BOOL`. The
+`LTYPE` descriptor numbers for other field types were renumbered.
+
+**Required actions:** Source code files must be recompiled, but regenerating
+`.pb.h`/`.pb.c` files from `.proto` is not required. If user code directly uses
+the nanopb internal field representation (search for `PB_LTYPE_VARINT` in source),
+it may need updating.
+
 Nanopb-0.3.9.1, 0.4.0 (2018-04-14)
 ==================================
 

docs/security.rst

@@ -58,6 +58,7 @@ untrusted data has been maliciously crafted:
    - The *count* fields of arrays will not exceed the array size.
    - The *size* field of bytes will not exceed the allocated size.
    - All string fields will have null terminator.
+   - bool fields will have valid true/false values (since nanopb-0.3.9.4)
 
 5. After pb_encode() returns successfully, the resulting message is a valid
    protocol buffers message. (Except if user-defined callbacks write incorrect

pb.h

@@ -164,39 +164,40 @@ typedef uint_least8_t pb_type_t;
 /**** Field data types ****/
 
 /* Numeric types */
-#define PB_LTYPE_VARINT  0x00 /* int32, int64, enum, bool */
-#define PB_LTYPE_UVARINT 0x01 /* uint32, uint64 */
-#define PB_LTYPE_SVARINT 0x02 /* sint32, sint64 */
-#define PB_LTYPE_FIXED32 0x03 /* fixed32, sfixed32, float */
-#define PB_LTYPE_FIXED64 0x04 /* fixed64, sfixed64, double */
+#define PB_LTYPE_BOOL    0x00 /* bool */
+#define PB_LTYPE_VARINT  0x01 /* int32, int64, enum, bool */
+#define PB_LTYPE_UVARINT 0x02 /* uint32, uint64 */
+#define PB_LTYPE_SVARINT 0x03 /* sint32, sint64 */
+#define PB_LTYPE_FIXED32 0x04 /* fixed32, sfixed32, float */
+#define PB_LTYPE_FIXED64 0x05 /* fixed64, sfixed64, double */
 
 /* Marker for last packable field type. */
-#define PB_LTYPE_LAST_PACKABLE 0x04
+#define PB_LTYPE_LAST_PACKABLE 0x05
 
 /* Byte array with pre-allocated buffer.
  * data_size is the length of the allocated PB_BYTES_ARRAY structure. */
-#define PB_LTYPE_BYTES 0x05
+#define PB_LTYPE_BYTES 0x06
 
 /* String with pre-allocated buffer.
  * data_size is the maximum length. */
-#define PB_LTYPE_STRING 0x06
+#define PB_LTYPE_STRING 0x07
 
 /* Submessage
  * submsg_fields is pointer to field descriptions */
-#define PB_LTYPE_SUBMESSAGE 0x07
+#define PB_LTYPE_SUBMESSAGE 0x08
 
 /* Extension pseudo-field
  * The field contains a pointer to pb_extension_t */
-#define PB_LTYPE_EXTENSION 0x08
+#define PB_LTYPE_EXTENSION 0x09
 
 /* Byte array with inline, pre-allocated byffer.
  * data_size is the length of the inline, allocated buffer.
  * This differs from PB_LTYPE_BYTES by defining the element as
  * pb_byte_t[data_size] rather than pb_bytes_array_t. */
-#define PB_LTYPE_FIXED_LENGTH_BYTES 0x09
+#define PB_LTYPE_FIXED_LENGTH_BYTES 0x0A
 
 /* Number of declared LTYPES */
-#define PB_LTYPES_COUNT 0x0A
+#define PB_LTYPES_COUNT 0x0B
 #define PB_LTYPE_MASK 0x0F
 
 /**** Field repetition rules ****/
@@ -715,7 +716,7 @@ struct pb_extension_s {
 #endif
 
 /* The mapping from protobuf types to LTYPEs is done using these macros. */
-#define PB_LTYPE_MAP_BOOL               PB_LTYPE_VARINT
+#define PB_LTYPE_MAP_BOOL               PB_LTYPE_BOOL
 #define PB_LTYPE_MAP_BYTES              PB_LTYPE_BYTES
 #define PB_LTYPE_MAP_DOUBLE             PB_LTYPE_FIXED64
 #define PB_LTYPE_MAP_ENUM               PB_LTYPE_VARINT

pb_decode.c

@@ -33,6 +33,7 @@ static bool checkreturn default_extension_decoder(pb_istream_t *stream, pb_exten
 static bool checkreturn decode_extension(pb_istream_t *stream, uint32_t tag, pb_wire_type_t wire_type, pb_field_iter_t *iter);
 static bool checkreturn find_extension_field(pb_field_iter_t *iter);
 static bool pb_message_set_to_defaults(pb_field_iter_t *iter);
+static bool checkreturn pb_dec_bool(pb_istream_t *stream, const pb_field_iter_t *field);
 static bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_iter_t *field);
 static bool checkreturn pb_dec_fixed(pb_istream_t *stream, const pb_field_iter_t *field);
 static bool checkreturn pb_dec_bytes(pb_istream_t *stream, const pb_field_iter_t *field);
@@ -385,6 +386,9 @@ static bool checkreturn decode_basic_field(pb_istream_t *stream, pb_field_iter_t
 {
     switch (PB_LTYPE(field->type))
     {
+        case PB_LTYPE_BOOL:
+            return pb_dec_bool(stream, field);
+
         case PB_LTYPE_VARINT:
         case PB_LTYPE_UVARINT:
         case PB_LTYPE_SVARINT:
@@ -1253,6 +1257,16 @@ void pb_release(const pb_msgdesc_t *fields, void *dest_struct)
 
 /* Field decoders */
 
+bool pb_decode_bool(pb_istream_t *stream, bool *dest)
+{
+    uint32_t value;
+    if (!pb_decode_varint32(stream, &value))
+        return false;
+
+    *(bool*)dest = (value != 0);
+    return true;
+}
+
 bool pb_decode_svarint(pb_istream_t *stream, pb_int64_t *dest)
 {
     pb_uint64_t value;
@@ -1317,6 +1331,11 @@ bool pb_decode_fixed64(pb_istream_t *stream, void *dest)
 }
 #endif
 
+static bool checkreturn pb_dec_bool(pb_istream_t *stream, const pb_field_iter_t *field)
+{
+    return pb_decode_bool(stream, (bool*)field->pData);
+}
+
 static bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_iter_t *field)
 {
     if (PB_LTYPE(field->type) == PB_LTYPE_UVARINT)

pb_decode.h

@@ -144,18 +144,21 @@ bool pb_decode_tag(pb_istream_t *stream, pb_wire_type_t *wire_type, uint32_t *ta
 /* Skip the field payload data, given the wire type. */
 bool pb_skip_field(pb_istream_t *stream, pb_wire_type_t wire_type);
 
-/* Decode an integer in the varint format. This works for bool, enum, int32,
+/* Decode an integer in the varint format. This works for enum, int32,
  * int64, uint32 and uint64 field types. */
 #ifndef PB_WITHOUT_64BIT
 bool pb_decode_varint(pb_istream_t *stream, uint64_t *dest);
 #else
 #define pb_decode_varint pb_decode_varint32
 #endif
 
-/* Decode an integer in the varint format. This works for bool, enum, int32,
+/* Decode an integer in the varint format. This works for enum, int32,
  * and uint32 field types. */
 bool pb_decode_varint32(pb_istream_t *stream, uint32_t *dest);
 
+/* Decode a bool value in varint format. */
+bool pb_decode_bool(pb_istream_t *stream, bool *dest);
+
 /* Decode an integer in the zig-zagged svarint format. This works for sint32
  * and sint64. */
 #ifndef PB_WITHOUT_64BIT

pb_encode.c

@@ -30,6 +30,7 @@ static bool checkreturn encode_extension_field(pb_ostream_t *stream, const pb_fi
 static bool checkreturn default_extension_encoder(pb_ostream_t *stream, const pb_extension_t *extension);
 static void *pb_const_cast(const void *p);
 static bool checkreturn pb_encode_varint_32(pb_ostream_t *stream, uint32_t low, uint32_t high);
+static bool checkreturn pb_enc_bool(pb_ostream_t *stream, const pb_field_iter_t *field);
 static bool checkreturn pb_enc_varint(pb_ostream_t *stream, const pb_field_iter_t *field);
 static bool checkreturn pb_enc_fixed(pb_ostream_t *stream, const pb_field_iter_t *field);
 static bool checkreturn pb_enc_bytes(pb_ostream_t *stream, const pb_field_iter_t *field);
@@ -102,6 +103,22 @@ bool checkreturn pb_write(pb_ostream_t *stream, const pb_byte_t *buf, size_t cou
  * Encode a single field *
  *************************/
 
+/* Read a bool value without causing undefined behavior even if the value
+ * is invalid. See issue #434 and
+ * https://stackoverflow.com/questions/27661768/weird-results-for-conditional
+ */
+static bool safe_read_bool(const void *pSize)
+{
+    const char *p = (const char *)pSize;
+    size_t i;
+    for (i = 0; i < sizeof(bool); i++)
+    {
+        if (p[i] != 0)
+            return true;
+    }
+    return false;
+}
+
 /* Encode a static array. Handles the size calculations and possible packing. */
 static bool checkreturn encode_array(pb_ostream_t *stream, pb_field_iter_t *field)
 {
@@ -241,7 +258,7 @@ static bool checkreturn pb_check_proto3_default_value(const pb_field_iter_t *fie
         else if (PB_HTYPE(type) == PB_HTYPE_OPTIONAL && field->pSize != NULL)
         {
             /* Proto2 optional fields inside proto3 submessage */
-            return *(const bool*)field->pSize == false;
+            return safe_read_bool(field->pSize) == false;
         }
 
         /* Rest is proto3 singular fields */
@@ -320,6 +337,9 @@ static bool checkreturn encode_basic_field(pb_ostream_t *stream, const pb_field_
 
     switch (PB_LTYPE(field->type))
     {
+        case PB_LTYPE_BOOL:
+            return pb_enc_bool(stream, field);
+
         case PB_LTYPE_VARINT:
         case PB_LTYPE_UVARINT:
         case PB_LTYPE_SVARINT:
@@ -627,6 +647,7 @@ bool pb_encode_tag_for_field ( pb_ostream_t* stream, const pb_field_iter_t* fiel
     pb_wire_type_t wiretype;
     switch (PB_LTYPE(field->type))
     {
+        case PB_LTYPE_BOOL:
         case PB_LTYPE_VARINT:
         case PB_LTYPE_UVARINT:
         case PB_LTYPE_SVARINT:
@@ -715,6 +736,13 @@ bool checkreturn pb_encode_submessage(pb_ostream_t *stream, const pb_msgdesc_t *
 
 /* Field encoders */
 
+static bool checkreturn pb_enc_bool(pb_ostream_t *stream, const pb_field_iter_t *field)
+{
+    uint32_t value = (uint32_t)safe_read_bool(field->pData);
+    PB_UNUSED(field);
+    return pb_encode_varint(stream, value);
+}
+
 static bool checkreturn pb_enc_varint(pb_ostream_t *stream, const pb_field_iter_t *field)
 {
     if (PB_LTYPE(field->type) == PB_LTYPE_UVARINT)