peter@adpm.de wrote:
> Full_Name: Peter Marschall
> Version: 2.4.42
> OS: Linux
> URL: https://github.com/marschap/openldap/tree/contrib-totp
> Submission from: (NULL) (92.211.7.6)
>
>
> Hi,
>
> I have written some bugfixes & flexibilizations for the TOTP contrib module.
>
> You can find them in the github branch:
>      https://github.com/marschap/openldap/tree/contrib-totp
>
> It differs from today's master by these commits:
> * https://github.com/marschap/openldap/commit/d67bffc4a361cecfce69fb4d14edb334d4e02c6a
>      contrib/passwd/totp: flexibilize decoding key
>
>      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)

>      - allow padding to be omitted (totally, not only parts)

Why?

>      In function chk_totp() determine the space required to hold the decoded
>      key by calling totp_b32_pton()  with a NULL argument for the target.
>
> * https://github.com/marschap/openldap/commit/435976d4f2468946bd0c5081ce7e2ae9fc0659fb
>      contrib/passwd/totp: fix the big-endian case
>
>      For the big-endian case, 'msg' wasn't set from 'tval' in generate().

This patch is wrong, as I commented on github.

> * https://github.com/marschap/openldap/commit/04c15b7b1e44d4d3167577702a8b77c5ee8bb7b6
>      contrib/passwd/totp: fix decoding when padding is used
>
>      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?

> * https://github.com/marschap/openldap/commit/04c15b7b1e44d4d3167577702a8b77c5ee8bb7b6
>      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.
>
> that change the file
>   contrib/slapd-modules/passwd/totp/slapd-totp.c | 67
> ++++++++++++++++++B%B++++++++++++++++++++++++++++++++++++------------
>
> I'd appreciate if you include them into OpenLDAP.
>
> The referenced patch files are derived from OpenLDAP Software.
> All of the modifications to OpenLDAP Software represented in the following
> patch(es) were developedy y Peter Marschall <peter@adpm.de>.
> I have not assigned rights and/or interest in this work to any party.
>
> The referenced modifications to OpenLDAP Software are subject to the following
> notice:
> Copyright 2015 Peter Marschall
> Redistribution and use in source and binary forms, with or without
> modification,
> are permitted only as authorized by the OpenLDAP Public License.
>
> Thanks in advance
> Peter
>
>


-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

On 29/08/15 17:44, hyc@symas.com wrote:
> This patch is incorrect. You must guard toupper(), it is not guaranteed not to
> distort non-lowercase characters.

Yes it is - for values in the range of unsigned char and EOF.
Behavior is undefined for other values.  The bug is failing to
cast a char arguments to <ctype.h> functions to unsigned char.

It's a common bug, so some implementations support char args.
They cannot support that fully for (char)EOF, though.

> 	if (islower(x)) x = toupper(x)

That might hide the bug at times, maybe that's why you use it.
But it's not reliable, islower too expects EOF or unsigned char.
Use x = toupper((unsigned char)x).

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

Peter Marschall wrote:

>>>       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?

That is exactly wrong. Given any integer input the string should be identical 
regardless of endianness. It should be "87654321" in both cases. With your 
patch, hashes will be incompatible between different machine architectures.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

Hi,

On Sunday, 30. August 2015 23:58:09 Howard Chu wrote:
> > 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?
> 
> That is exactly wrong. Given any integer input the string should be
> identical regardless of endianness. It should be "87654321" in both cases.
> With your patch, hashes will be incompatible between different machine
> architectures.

Please read my mail again!
I tested the WORDS_BIGENDIAN case on a *little-endian* architecture using the 
same data as the !WORDS_BIGENDIAN case.

In that case [i.e. testing on the "wrong-endian" architecture] the result *is 
expected* to come out in reversed order.
Otherwise it would end-up in wrong order on the "right-endian" architecture.

Do you agree?

If you don't, please hint me to some code that allows me to check and fix my 
code.

Best
Peter

-- 
Peter Marschall
peter@adpm.de

Peter Marschall wrote:
> Hi,
>
> On Sunday, 30. August 2015 23:58:09 Howard Chu wrote:
>>> 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?
>>
>> That is exactly wrong. Given any integer input the string should be
>> identical regardless of endianness. It should be "87654321" in both cases.
>> With your patch, hashes will be incompatible between different machine
>> architectures.
>
> Please read my mail again!
> I tested the WORDS_BIGENDIAN case on a *little-endian* architecture using the
> same data as the !WORDS_BIGENDIAN case.
>
> In that case [i.e. testing on the "wrong-endian" architecture] the result *is
> expected* to come out in reversed order.
> Otherwise it would end-up in wrong order on the "right-endian" architecture.
>
> Do you agree?

The point is that your patch *always* reverses the order of the bytes. On a 
big-endian machine the code should be a no-op.
>
> If you don't, please hint me to some code that allows me to check and fix my
> code.
>
> Best
> Peter
>


-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

Hi,

Please have a look at the improved patch series in
	https://github.com/marschap/openldap/commits/contrib-totp2
It tries to address all the issues highlighted on the first version.

https://github.com/marschap/openldap/commit/e4e1045f59368af8d15172ad22fcc0fd1d99a28b
	contrib/passwd/totp: flexibilize decoding
    
	In function totp_b32_pton()
	- allow lowercase characters in encoded string too
	- allow padding to be omitted (totally, not only parts)
	With this added flexibility we can make use of keys encoded
	by other Base32-encoding implementations.
    
	In function chk_totp() determine the space required to hold the decoded
	key by calling totp_b32_pton() with a NULL argument for the target.

    Changes to previous version:
	- toupper()'s argument guarded with a cast to (unsigned char)
	- added rationale to commit message

https://github.com/marschap/openldap/commit/edfa2b0fb3238ca9f231fa75bd452b4221f9431d
	contrib/passwd/totp: fix the big-endian case, support 32-bit archs
    
	- reverse tval in the WORDS_BIGENDIAN case before converting it to a
	  string
	- use uint64_t for tval to have it correctly sized on 32-bit archs too	
	- avoid magic number when converting tval to a string

    Changes to previous version: 
	- complete rewrite

https://github.com/marschap/openldap/commit/24007f02cebb0b7f801288b02d0ac8c2f1d4ea05
	contrib/passwd/totp: fix decoding when padding is used
    
	In totp_b32_pton(), correctly count the number of '=' padding chars
	at the end of the base-32 encoded string: don't count the first
	padding char char twice.

	Note: '*str++' evaluates *str first and increases str later!

    Changes to previous version:
	- commit message only

https://github.com/marschap/openldap/commit/435619ccd8be1b62f86db67643bca7775ead65dc
	contrib/passwd/totp: support compiling using nettle

    Changes to previous version:
	- rewrite, concentrating the #ifdef's as much as possible

Best
Peter

-- 
Peter Marschall
peter@adpm.de

Hi,
 
Please have a look at the improved patch series in
 	https://github.com/marschap/openldap/commits/contrib-totp3
and
 	https://github.com/marschap/openldap/commits/contrib-totp4
They try to address the issues in the previous versions

Changes:
*	contrib/passwd/totp: fix the big-endian case, support 32-bit archs
	Use memcpy for the WORDS_BIGENDIAN case [much simpler]
	Use uint64_t for same results in 32- and 64-bit cases

*	contrib/passwd/totp: support compiling using nettle
	Fix the botched previous patch.
	The two branches differ only in that patch, giving the following
	alternatives:
	- contrib-totp3 continues to use the HMAC_... macros 
	  It still requires the #if...#else...#endif to harmonize the setup
	  in do_hmac()
	- contrib-totp4 gets rid of the HMAC_... macros
	  It uses the different implementations directly in do_hmac()
	  Of course separated by #if...#else..#endif
	Functionally both patches are equivalent: 
	in my tests they yield the same results for OpenSSL and GnuTLS.
	  
Best
PEter

-- 
Peter Marschall
peter@adpm.de

peter@adpm.de wrote:
>>>       - 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)

We must reject this on security grounds. See RFC3548 Security Considerations. 
https://tools.ietf.org/html/rfc3548#page-10

Also, as already noted in the code comments, allowing partial bytes would open 
a subliminal channel allowing information leaks.

If Perl's encoder is being so careless then that is a security vulnerability.

The other 3 points on this ticket have been committed in master. I consider 
this ticket resolved.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/

fixed in master

changed notes
changed state Open to Test
moved from Incoming to Software Bugs