Fix undefined behavior with bool fields (#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

6 files changed, 82 insertions, 17 deletions

diff --git a/docs/migration.rst b/docs/migration.rst
index a1843e3..d358600 100644
--- a/docs/migration.rst
+++ b/docs/migration.rst
@@ -28,6 +28,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)
 ==================================
 
diff --git a/docs/security.rst b/docs/security.rst
index d854612..6f7152e 100644
--- a/docs/security.rst
+++ b/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
diff --git a/pb.h b/pb.h
index 5a3ebb8..aaf3f58 100644
--- a/pb.h
+++ b/pb.h
@@ -150,39 +150,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 ****/
@@ -491,7 +492,7 @@ struct pb_extension_s {
     PB_OPTIONAL_CALLBACK(tag, st, m, fd, ltype, ptr)
 
 /* 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
diff --git a/pb_decode.c b/pb_decode.c
index a7caeff..1779503 100644
--- a/pb_decode.c
+++ b/pb_decode.c
@@ -34,6 +34,7 @@ static bool checkreturn decode_extension(pb_istream_t *stream, uint32_t tag, pb_
 static bool checkreturn find_extension_field(pb_field_iter_t *iter);
 static void pb_field_set_to_default(pb_field_iter_t *iter);
 static void pb_message_set_to_defaults(const pb_field_t fields[], void *dest_struct);
+static bool checkreturn pb_dec_bool(pb_istream_t *stream, const pb_field_t *field, void *dest);
 static bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_t *field, void *dest);
 static bool checkreturn pb_decode_varint32_eof(pb_istream_t *stream, uint32_t *dest, bool *eof);
 static bool checkreturn pb_dec_uvarint(pb_istream_t *stream, const pb_field_t *field, void *dest);
@@ -65,6 +66,7 @@ static void pb_release_single_field(const pb_field_iter_t *iter);
  * Order in the array must match pb_action_t LTYPE numbering.
  */
 static const pb_decoder_t PB_DECODERS[PB_LTYPES_COUNT] = {
+    &pb_dec_bool,
     &pb_dec_varint,
     &pb_dec_uvarint,
     &pb_dec_svarint,
@@ -1245,6 +1247,11 @@ void pb_release(const pb_field_t fields[], void *dest_struct)
 
 /* Field decoders */
 
+bool pb_decode_bool(pb_istream_t *stream, bool *dest)
+{
+    return pb_dec_bool(stream, NULL, (void*)dest);
+}
+
 bool pb_decode_svarint(pb_istream_t *stream, pb_int64_t *dest)
 {
     pb_uint64_t value;
@@ -1294,6 +1301,17 @@ bool pb_decode_fixed64(pb_istream_t *stream, void *dest)
 }
 #endif
 
+static bool checkreturn pb_dec_bool(pb_istream_t *stream, const pb_field_t *field, void *dest)
+{
+    uint32_t value;
+    PB_UNUSED(field);
+    if (!pb_decode_varint32(stream, &value))
+        return false;
+
+    *(bool*)dest = (value != 0);
+    return true;
+}
+
 static bool checkreturn pb_dec_varint(pb_istream_t *stream, const pb_field_t *field, void *dest)
 {
     pb_uint64_t value;
diff --git a/pb_decode.h b/pb_decode.h
index 398b24a..3577c20 100644
--- a/pb_decode.h
+++ b/pb_decode.h
@@ -134,7 +134,7 @@ 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);
@@ -142,10 +142,13 @@ bool pb_decode_varint(pb_istream_t *stream, uint64_t *dest);
 #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
diff --git a/pb_encode.c b/pb_encode.c
index 64b62b4..a8cd14e 100644
--- a/pb_encode.c
+++ b/pb_encode.c
@@ -28,6 +28,7 @@ static bool checkreturn encode_field(pb_ostream_t *stream, const pb_field_t *fie
 static bool checkreturn default_extension_encoder(pb_ostream_t *stream, const pb_extension_t *extension);
 static bool checkreturn encode_extension_field(pb_ostream_t *stream, const pb_field_t *field, const void *pData);
 static void *pb_const_cast(const void *p);
+static bool checkreturn pb_enc_bool(pb_ostream_t *stream, const pb_field_t *field, const void *src);
 static bool checkreturn pb_enc_varint(pb_ostream_t *stream, const pb_field_t *field, const void *src);
 static bool checkreturn pb_enc_uvarint(pb_ostream_t *stream, const pb_field_t *field, const void *src);
 static bool checkreturn pb_enc_svarint(pb_ostream_t *stream, const pb_field_t *field, const void *src);
@@ -52,6 +53,7 @@ static bool checkreturn pb_encode_negative_varint(pb_ostream_t *stream, pb_uint6
  * Order in the array must match pb_action_t LTYPE numbering.
  */
 static const pb_encoder_t PB_ENCODERS[PB_LTYPES_COUNT] = {
+    &pb_enc_bool,
     &pb_enc_varint,
     &pb_enc_uvarint,
     &pb_enc_svarint,
@@ -122,6 +124,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, const pb_field_t *field,
                          const void *pData, size_t count, pb_encoder_t func)
@@ -240,7 +258,7 @@ static bool pb_check_proto3_default_value(const pb_field_t *field, const void *p
     else if (PB_HTYPE(type) == PB_HTYPE_OPTIONAL && field->size_offset != 0)
     {
         /* Proto2 optional fields inside proto3 submessage */
-        return *(const bool*)pSize == false;
+        return safe_read_bool(pSize) == false;
     }
 
     /* Rest is proto3 singular fields */
@@ -356,7 +374,7 @@ static bool checkreturn encode_basic_field(pb_ostream_t *stream,
             break;
         
         case PB_HTYPE_OPTIONAL:
-            if (*(const bool*)pSize)
+            if (safe_read_bool(pSize))
             {
                 if (!pb_encode_tag_for_field(stream, field))
                     return false;
@@ -648,6 +666,7 @@ bool checkreturn pb_encode_tag_for_field(pb_ostream_t *stream, const pb_field_t
     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:
@@ -736,6 +755,13 @@ bool checkreturn pb_encode_submessage(pb_ostream_t *stream, const pb_field_t fie
 
 /* Field encoders */
 
+static bool checkreturn pb_enc_bool(pb_ostream_t *stream, const pb_field_t *field, const void *src)
+{
+    uint32_t value = (uint32_t)safe_read_bool(src);
+    PB_UNUSED(field);
+    return pb_encode_varint(stream, value);
+}
+
 static bool checkreturn pb_enc_varint(pb_ostream_t *stream, const pb_field_t *field, const void *src)
 {
     pb_int64_t value = 0;