Re: (ITS#8230) [PATCH] totp: bug fixes and improvements

[peter@adpm.de]

Hi Howard,

thanks for the quick response.

On Saturday, 29. August 2015 16:43:59 Howard Chu wrote:
> >      In function totp_b32_pton()
> >      - allow lowercase characters in encoded string too
> 
> This patch is incorrect. You must guard toupper(), it is not guaranteed not
> to distort non-lowercase characters.
> 
> 	if (islower(x)) x = toupper(x)
Thx, will include it in a reworked version.

> >      - allow padding to be omitted (totally, not only parts)
> 
> Why?
To allow using the keys encoded by other implementations that do
not generate the padding (e.g. Perl's Convert::Base32).
(e.g. in a mass-rollout that sets userPassword using LDIF)


> >      For the big-endian case, 'msg' wasn't set from 'tval' in generate().
> 
> This patch is wrong, as I commented on github.
Are you sure?

Please note the run index i changed direction, which causes different results
because of  >>=.

In my tests [double checked before sending this mail] on an amd64 [little-
endian architecture], the patched version converts 0x3837363534333231 to
- "87654321"	in the !WORDS_BIGENDIAN case [unchanged]
- "12345678"	in the WORDS_BIGENDIAN case
[using the fact that  0x3N = "N"  for 0 <= N <=9]
To me that looks correct.

What issues did you see exactly?

> >      In totp_b32_pton(), correctly count the number of '=' padding chars
> >      at the end of the base-32 encoded string.
> >      
> >      Note: '*str++' evaluates *str first and increases str later!
> 
> The current code correctly decodes known test data. What is your test case?
Hmm, strange.

On my 64-bit Linux the original implementation failed to decode
keys encoded by the original implementation's encoder.

It took me quite some time to find the exact cause.
I tested my patch with the official test vectors of chapter 10 of RFC 4648.

With my patch the decoding in totp_b32_pton() is symmetric to encoding 
performed by totp_b32_ntop(), i.e. encoded strings decode to the original.

In addition testing reproduced the official RFC4648 test cases in both 
directions, where the original version miscalculated the number of padding 
bytes so that  "state + i" did not correctly add up to 8 as required for the 
"good" cases.

I double checked before sending this mail: 
The original implementation fails to decode all test cases that have padding.
E.g. BASE32("foo") = "MZXW6===" [RFC4648]
In this example it counts the number of passing chars "=" to i = 4,
which I do not consider correct.

Would you mind posting your test cases?

BTW: not insisting on padding would help here too ;-)

> > *
> > https://github.com/marschap/openldap/commit/04c15b7b1e44d4d3167577702a8b7
> > 7c5ee8bb7b6> 
> >      contrib/passwd/totp: support compiling using nettle
> 
> The proper way to add this support is to add macros to hide all of the
> differences in the initial ifdef block and not to add any additional ifdefs
> anywhere in the main body of the code.
I'll have a closer look.

This patch tried to not touch the original OpenSSL version.
With a more "homogenous patch" I'll need to make some slight changes
to the macros for the OpenSSL version.
I hope that's OK.

Best
PEter

-- 
Peter Marschall
peter@adpm.de