Merge pull request #5 from int08h/fuzz

Land validation and error handling improvements from roughenough-fuzz

6 files changed, 130 insertions, 19 deletions

diff --git a/README.md b/README.md
index f4a444a..5c181d6 100644
--- a/README.md
+++ b/README.md
@@ -96,8 +96,6 @@ Roughtime features not implemented by the server:
 
 Other notes:
 
-* Error-handling needs a closer examination to verify the `unwrap()`'s and `expect()`'s present
-  in the request handling path are for truly exceptional conditions.
 * Per-request heap allocations could probably be reduced: a few `Vec`'s could be replaced by 
   lifetime scoped slices.
 
diff --git a/src/error.rs b/src/error.rs
index 98f337e..5833f2d 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -25,12 +25,27 @@ pub enum Error {
     /// The associated byte sequence does not correspond to a valid Roughtime tag.
     InvalidTag(Box<[u8]>),
 
+    /// Invalid number of tags specified
+    InvalidNumTags(u32),
+
+    /// Tag value length exceeds length of source bytes
+    InvalidValueLength(Tag, u32),
+
     /// Encoding failed. The associated `std::io::Error` should provide more information.
     EncodingFailure(std::io::Error),
 
     /// Request was less than 1024 bytes
     RequestTooShort,
 
+    /// Offset was not 32-bit aligned
+    InvalidAlignment(u32),
+
+    /// Offset is outside of valid message range
+    InvalidOffsetValue(u32),
+
+    /// Could not convert bytes to message because bytes were too short
+    MessageTooShort,
+
     /// Otherwise invalid request
     InvalidRequest,
 }
diff --git a/src/lib.rs b/src/lib.rs
index 4814398..2ef2c34 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -22,7 +22,7 @@
 //!
 //! # Protocol
 //!
-//! Roughtime messages are represetned by [`RtMessage`](struct.RtMessage.html) which
+//! Roughtime messages are represented by [`RtMessage`](struct.RtMessage.html) which
 //! implements the mapping of Roughtime `u32` [`tags`](enum.Tag.html) to byte-strings.
 //!
 //! # Client
diff --git a/src/merkle.rs b/src/merkle.rs
index d55280f..ba52f96 100644
--- a/src/merkle.rs
+++ b/src/merkle.rs
@@ -12,6 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//!
+//! Merkle Tree implementation using SHA-512 and the Roughtime leaf and node tweak values.
+//!
+
 extern crate ring;
 
 use super::{HASH_LENGTH, TREE_LEAF_TWEAK, TREE_NODE_TWEAK};
@@ -20,11 +24,18 @@ use self::ring::digest;
 type Data = Vec<u8>;
 type Hash = Data;
 
+///
+/// Merkle Tree implementation using SHA-512 and the Roughtime leaf and node tweak values.
+///
 pub struct MerkleTree {
     levels: Vec<Vec<Data>>,
 }
 
 impl MerkleTree {
+
+    ///
+    /// Create a new empty Merkle Tree
+    ///
     pub fn new() -> MerkleTree {
         MerkleTree {
             levels: vec![vec![]],
diff --git a/src/message.rs b/src/message.rs
index 11cad2b..7e1175b 100644
--- a/src/message.rs
+++ b/src/message.rs
@@ -43,38 +43,79 @@ impl RtMessage {
         }
     }
 
+    /// Construct a new RtMessage from the on-the-wire representation in `bytes`
+    ///
+    /// ## Arguments
+    ///
+    /// * `bytes` - On-the-wire representation
+    ///
     pub fn from_bytes(bytes: &[u8]) -> Result<Self, Error> {
-        let mut msg = Cursor::new(bytes);
+        let bytes_len = bytes.len();
 
-        let num_tags = msg.read_u32::<LittleEndian>()?;
-        let mut rt_msg = RtMessage::new(num_tags);
+        if bytes_len < 4 {
+            return Err(Error::MessageTooShort);
+        } else if bytes_len % 4 != 0 {
+            return Err(Error::InvalidAlignment(bytes_len as u32));
+        }
 
-        if num_tags == 1 {
-            let pos = msg.position() as usize;
-            let tag = Tag::from_wire(&bytes[pos..pos + 4])?;
-            msg.set_position((pos + 4) as u64);
+        let mut msg = Cursor::new(bytes);
+        let num_tags = msg.read_u32::<LittleEndian>()?;
 
-            let mut value = Vec::new();
+        match num_tags {
+            0 => Ok(RtMessage::new(0)),
+            1 => RtMessage::single_tag_message(bytes, &mut msg),
+            _ => RtMessage::multi_tag_message(num_tags, bytes, &mut msg),
+        }
+    }
 
-            msg.read_to_end(&mut value).unwrap();
-            rt_msg.add_field(tag, &value)?;
-            return Ok(rt_msg);
+    /// Internal function to create a single tag message
+    fn single_tag_message(bytes: &[u8], msg: &mut Cursor<&[u8]>) -> Result<Self, Error> {
+        if bytes.len() < 8 {
+            return Err(Error::MessageTooShort);
         }
 
+        let pos = msg.position() as usize;
+        msg.set_position((pos + 4) as u64);
+
+        let mut value = Vec::new();
+        msg.read_to_end(&mut value)?;
+
+        let tag = Tag::from_wire(&bytes[pos..pos + 4])?;
+        let mut rt_msg = RtMessage::new(1);
+        rt_msg.add_field(tag, &value)?;
+
+        Ok(rt_msg)
+    }
+
+    /// Internal function to create a multiple tag message
+    fn multi_tag_message(
+        num_tags: u32,
+        bytes: &[u8],
+        msg: &mut Cursor<&[u8]>,
+    ) -> Result<Self, Error> {
+        let bytes_len = bytes.len();
         let mut offsets = Vec::with_capacity((num_tags - 1) as usize);
-        let mut tags = Vec::with_capacity(num_tags as usize);
 
         for _ in 0..num_tags - 1 {
             let offset = msg.read_u32::<LittleEndian>()?;
+
             if offset % 4 != 0 {
-                panic!("Invalid offset {:?} in message {:?}", offset, bytes);
+                return Err(Error::InvalidAlignment(offset));
+            } else if offset > bytes_len as u32 {
+                return Err(Error::InvalidOffsetValue(offset));
             }
+
             offsets.push(offset as usize);
         }
 
         let mut buf = [0; 4];
+        let mut tags = Vec::with_capacity(num_tags as usize);
+
         for _ in 0..num_tags {
-            msg.read_exact(&mut buf).unwrap();
+            if msg.read_exact(&mut buf).is_err() {
+                return Err(Error::MessageTooShort);
+            }
+
             let tag = Tag::from_wire(&buf)?;
 
             if let Some(last_tag) = tags.last() {
@@ -82,26 +123,36 @@ impl RtMessage {
                     return Err(Error::TagNotStrictlyIncreasing(tag));
                 }
             }
+
             tags.push(tag);
         }
 
         // All offsets are relative to the end of the header,
         // which is our current position
         let header_end = msg.position() as usize;
+
         // Compute the end of the last value,
         // as an offset from the end of the header
         let msg_end = bytes.len() - header_end;
 
-        assert_eq!(offsets.len(), tags.len() - 1);
+        let mut rt_msg = RtMessage::new(num_tags);
 
         for (tag, (value_start, value_end)) in tags.into_iter().zip(
             once(&0)
                 .chain(offsets.iter())
                 .zip(offsets.iter().chain(once(&msg_end))),
         ) {
-            let value = bytes[(header_end + value_start)..(header_end + value_end)].to_vec();
+            let start_idx = header_end + value_start;
+            let end_idx = header_end + value_end;
+
+            if end_idx > msg_end || start_idx > end_idx {
+                return Err(Error::InvalidValueLength(tag, end_idx as u32));
+            }
+
+            let value = bytes[start_idx..end_idx].to_vec();
             rt_msg.add_field(tag, &value)?;
         }
+
         Ok(rt_msg)
     }
 
@@ -133,14 +184,17 @@ impl RtMessage {
         self.tags.len() as u32
     }
 
+    /// Returns a slice of the tags in the message
     pub fn tags(&self) -> &[Tag] {
         &self.tags
     }
 
+    /// Returns a slice of the values in the message
     pub fn values(&self) -> &[Vec<u8>] {
         &self.values
     }
 
+    /// Converts the message into a `HashMap` mapping each tag to its value
     pub fn into_hash_map(self) -> HashMap<Tag, Vec<u8>> {
         self.tags.into_iter().zip(self.values.into_iter()).collect()
     }
@@ -339,4 +393,35 @@ mod test {
         // Everything was read
         assert_eq!(encoded.position() as usize, msg.encoded_size());
     }
+
+    #[test]
+    fn from_bytes_zero_tags() {
+        let bytes = [0, 0, 0, 0];
+        let msg = RtMessage::from_bytes(&bytes).unwrap();
+
+        assert_eq!(msg.num_fields(), 0);
+    }
+
+    #[test]
+    #[should_panic(expected = "InvalidAlignment")]
+    fn from_bytes_offset_past_end_of_message() {
+        let mut msg = RtMessage::new(2);
+        msg.add_field(Tag::NONC, "1111".as_bytes()).unwrap();
+        msg.add_field(Tag::PAD, "aaaaaaaaa".as_bytes()).unwrap();
+
+        let mut bytes = msg.encode().unwrap();
+        // set the PAD value offset to beyond end of the message
+        bytes[4] = 128;
+
+        RtMessage::from_bytes(&bytes).unwrap();
+    }
+
+    #[test]
+    #[should_panic(expected = "InvalidAlignment")]
+    fn from_bytes_too_few_bytes_for_tags() {
+        // Header says two tags (8 bytes) but truncate first tag at 2 bytes
+        let bytes = &[0x02, 0, 0, 0, 4, 0, 0, 0, 0, 0];
+        RtMessage::from_bytes(bytes).unwrap();
+    }
+
 }
diff --git a/src/tag.rs b/src/tag.rs
index 2b9c1f4..34223df 100644
--- a/src/tag.rs
+++ b/src/tag.rs
@@ -59,6 +59,8 @@ impl Tag {
         }
     }
 
+    /// Return the `Tag` corresponding to the on-the-wire representation in `bytes` or an
+    /// `Error::InvalidTag` if `bytes` do not correspond to a valid tag.
     pub fn from_wire(bytes: &[u8]) -> Result<Self, Error> {
         match bytes {
             b"CERT" => Ok(Tag::CERT),