Merge pull request #335 from defuse/strip-newlines

Strip Newlines from ASCII-Safe Strings
defuse · Apr 21, 2017 · 0364e3e · 0364e3e
2 parents 3787bc0 + 40640d0
commit 0364e3e
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 2 deletions.
diff --git a/docs/classes/Key.md b/docs/classes/Key.md
@@ -74,17 +74,23 @@ None.
 
 None.
 
-### Key::loadFromAsciiSafeString($saved\_key\_string)
+### Key::loadFromAsciiSafeString($saved\_key\_string, $do\_not\_trim = false)
 
 **Description:**
 
 Loads an instance of `Key` that was saved to a string by
 `saveToAsciiSafeString()`.
 
+By default, this function will call `Encoding::trimTrailingWhitespace()`
+to remove trailing CR, LF, NUL, TAB, and SPACE characters, which are commonly
+appended to files when working with text editors.
+
 **Parameters:**
 
 1. `$saved_key_string` is the string returned from `saveToAsciiSafeString()`
    when the original `Key` instance was saved.
+2. `$do_not_trim` should be set to `TRUE` if you do not wish for the library
+   to automatically strip trailing whitespace from the string. 
 
 **Return value:**
 

diff --git a/src/Encoding.php b/src/Encoding.php
@@ -77,6 +77,62 @@ public static function hexToBin($hex_string)
         }
         return $bin;
     }
+
+    /**
+     * Remove trialing whitespace without table look-ups or branches.
+     *
+     * Calling this function may leak the length of the string as well as the
+     * number of trailing whitespace characters through side-channels.
+     *
+     * @param string $string
+     * @return string
+     */
+    public static function trimTrailingWhitespace($string = '')
+    {
+        $length = Core::ourStrlen($string);
+        if ($length < 1) {
+            return '';
+        }
+        do {
+            $prevLength = $length;
+            $last = $length - 1;
+            $chr = \ord($string[$last]);
+
+            /* Null Byte (0x00), a.k.a. \0 */
+            // if ($chr === 0x00) $length -= 1;
+            $sub = (($chr - 1) >> 8 ) & 1;
+            $length -= $sub;
+            $last -= $sub;
+
+            /* Horizontal Tab (0x09) a.k.a. \t */
+            $chr = \ord($string[$last]);
+            // if ($chr === 0x09) $length -= 1;
+            $sub = (((0x08 - $chr) & ($chr - 0x0a)) >> 8) & 1;
+            $length -= $sub;
+            $last -= $sub;
+
+            /* New Line (0x0a), a.k.a. \n */
+            $chr = \ord($string[$last]);
+            // if ($chr === 0x0a) $length -= 1;
+            $sub = (((0x09 - $chr) & ($chr - 0x0b)) >> 8) & 1;
+            $length -= $sub;
+            $last -= $sub;
+
+            /* Carriage Return (0x0D), a.k.a. \r */
+            $chr = \ord($string[$last]);
+            // if ($chr === 0x0d) $length -= 1;
+            $sub = (((0x0c - $chr) & ($chr - 0x0e)) >> 8) & 1;
+            $length -= $sub;
+            $last -= $sub;
+
+            /* Space */
+            $chr = \ord($string[$last]);
+            // if ($chr === 0x20) $length -= 1;
+            $sub = (((0x1f - $chr) & ($chr - 0x21)) >> 8) & 1;
+            $length -= $sub;
+        } while ($prevLength !== $length && $length > 0);
+        return Core::ourSubstr($string, 0, $length);
+    }
 
     /*
      * SECURITY NOTE ON APPLYING CHECKSUMS TO SECRETS:
@@ -161,6 +217,8 @@ public static function loadBytesFromChecksummedAsciiSafeString($expected_header,
             );
         }
 
+        /* If you get an exception here when attempting to load from a file, first pass your
+           key to Encoding::trimTrailingWhitespace() to remove newline characters, etc.      */
         $bytes = Encoding::hexToBin($string);
 
         /* Make sure we have enough bytes to get the version header and checksum. */

diff --git a/src/Key.php b/src/Key.php
@@ -26,15 +26,23 @@ public static function createNewRandomKey()
     /**
      * Loads a Key from its encoded form.
      *
+     * By default, this function will call Encoding::trimTrailingWhitespace()
+     * to remove trailing CR, LF, NUL, TAB, and SPACE characters, which are
+     * commonly appended to files when working with text editors.
+     *
      * @param string $saved_key_string
+     * @param bool $do_not_trim (default: false)
      *
      * @throws Ex\BadFormatException
      * @throws Ex\EnvironmentIsBrokenException
      *
      * @return Key
      */
-    public static function loadFromAsciiSafeString($saved_key_string)
+    public static function loadFromAsciiSafeString($saved_key_string, $do_not_trim = false)
     {
+        if (!$do_not_trim) {
+            $saved_key_string = Encoding::trimTrailingWhitespace($saved_key_string);
+        }
         $key_bytes = Encoding::loadBytesFromChecksummedAsciiSafeString(self::KEY_CURRENT_VERSION, $saved_key_string);
         return new Key($key_bytes);
     }

diff --git a/test/unit/EncodingTest.php b/test/unit/EncodingTest.php
@@ -82,4 +82,30 @@ public function testBadHexEncoding()
         $str[0] = 'Z';
         Encoding::loadBytesFromChecksummedAsciiSafeString($header, $str);
     }
+
+    /**
+     * This shouldn't throw an exception.
+     */
+    public function testPaddedHexEncoding()
+    {
+        /* We're just ensuring that an empty string doesn't produce an error. */
+        $this->assertSame('', Encoding::trimTrailingWhitespace(''));
+
+        $header = Core::secureRandom(Core::HEADER_VERSION_SIZE);
+        $str = Encoding::saveBytesToChecksummedAsciiSafeString(
+            $header,
+            Core::secureRandom(Core::KEY_BYTE_SIZE)
+        );
+        $orig = $str;
+        $noise = ["\r", "\n", "\t", "\0"];
+        for ($i = 0; $i < 1000; ++$i) {
+            $c = $noise[\random_int(0, 3)];
+            $str .= $c;
+            $this->assertSame(
+                Encoding::binToHex($orig),
+                Encoding::binToHex(Encoding::trimTrailingWhitespace($str)),
+                'Pass #' . $i . ' (' . \dechex(\ord($c)) . ')'
+            );
+        }
+    }
 }