bug#7155: [md5sum] does not accept

[Pádraig Brady]

severity 7155 wishlist
tags 7155 + notabug

Comments below.

On 10/16/2010 11:37 PM, Pádraig Brady wrote:
> On 16/10/10 20:37, Rimas Kudelis wrote:
>>
>> On Sun, 03 Oct 2010 23:27:24 +0100, Pádraig Brady <address@hidden>
>> wrote:
>>> On 03/10/10 20:24, Rimas Kudelis wrote:
>>>> Hi,
>>>>
>>>> I have a little problem with md5sum.
>>>>
>>>> A FreeBSD box generates an md5 sum of a file, which I'm later trying to
>>>> check on a Linux box. The problem is that what FreeBSD's md5 outputs is
>>>> slightly different from what Linux's md5sum expects, which makes md5sum
>>>> complain. The difference is really trivial: md5 outputs one space
>>>> between the sum and the file name, and md5sum outputs/expects two:
>>>
>>> md5 seems to output a different format here.
>>>
>>> $ head -n1 /etc/motd
>>> FreeBSD 8.0-RELEASE-p3 (GENERIC) #0: Wed May 26 05:45:12 UTC 2010
>>> $ md5sum --version | head -n1
>>> md5sum (GNU coreutils) 8.3
>>> $ md5 file | tee t.md5
>>> MD5 (file) = b85d6fb9ef4260dcf1ce0a1b0bff80d3
>>> $ md5sum -c t.md5
>>> file: OK
>>>
>>> Could you verify what md5 utility you're using exactly.
>>
>>
>> Sorry for taking so long to answer, but I wasn't the person producing the
>> checksum, so I had to ask too. The command used to produce the checksum is:
>> $ md5 -r <filename>
>>
>> FreeBSD release version is the same as yours. I've just tested the same
>> command with FreeBSD 6.2, and it only outputs one space too.
> 
> Ah right, md5 -r produces the alternate format.
> I suppose we could support a single space,
> by trying to open("*abc") after trying to open ("abc").
> There is still an ambiguity if both files are present,
> though that is unlikely. I'll have a look.

Thinking more about this, there is a bit of a
security issue with mixing both formats.
Consider the case currently with a checksum in BSD format
of ' important_file' (with leading space).

b85d6fb9ef4260dcf1ce0a1b0bff80d3  important_file

Now an attacker does:

mv ' important_file' important_file
cp trojan ' important_file'

And since coreutils will check 'important_file' first,
then we'll not report any errors.

If coreutils supported both formats then this would be compounded.
I suppose we could mitigate that somewhat by detecting and
supporting only a single format per run, as done in the following diff.

Note we don't handle the case where the first or only
entry in a BSD format checksum file has a file name with a
leading ' ' or '*'.  We could support this and avoid detection
with an option to specify BSD format checksums, but
I don't think that's warranted. Note we could detect
in this situation too by retrying the open with the
leading char included, but that would introduce a security
issue. Consider the following standard format checksum file:

b85d6fb9ef4260dcf1ce0a1b0bff80d3  firewall_rules

Attackers could then do this undetected:

mv firewall_rules ' firewall_rules'

cheers,
Pádraig.

diff --git a/src/md5sum.c b/src/md5sum.c
index ff9538a..9de34a5 100644
--- a/src/md5sum.c
+++ b/src/md5sum.c
@@ -99,7 +99,7 @@
    not include any newline character at the end of a line.  */
 #define MIN_DIGEST_LINE_LENGTH \
   (DIGEST_HEX_BYTES /* length of hexadecimal message digest */ \
-   + 2 /* blank and binary indicator */ \
+   + 1 /* blank */ \
    + 1 /* minimum filename length */ )

 /* True if any of the files read were the standard input. */
@@ -126,6 +126,9 @@ static bool quiet = false;
    improperly formatted. */
 static bool strict = false;

+/* Whether a BSD reversed format checksum is detected.  */
+static int bsd_reversed = -1;
+
 /* For long options that have no equivalent short option, use a
    non-character as a pseudo short option, starting with CHAR_MAX + 1.  */
 enum
@@ -307,9 +310,24 @@ split_3 (char *s, size_t s_len,

   s[i++] = '\0';

-  if (s[i] != ' ' && s[i] != '*')
-    return false;
-  *binary = (s[i++] == '*');
+  /* If "bsd reversed" format detected.  */
+  if ((s_len - i == 1) || (s[i] != ' ' && s[i] != '*'))
+    {
+      /* Don't allow mixing bsd and standard formats,
+         to minimize security issues with attackers
+         renaming files with leading spaces.
+         This assumes that with bsd format checksums
+         that the first file name does not have
+         a leading ' ' or '*'.  */
+      if (bsd_reversed == 0)
+        return false;
+      bsd_reversed = 1;
+    }
+  else if (bsd_reversed != 1)
+    {
+      bsd_reversed = 0;
+      *binary = (s[i++] == '*');
+    }

   /* All characters between the type indicator and end of line are
      significant -- that includes leading and trailing white space.  */