openldap-bugs August 2015

openldap-bugs@openldap.org

24 participants
69 discussions

Re: (ITS#8230) [PATCH] totp: bug fixes and improvements
by hyc＠symas.com 01 Sep '15

01 Sep '15

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/

1 0

Re: (ITS#8230) [PATCH] totp: bug fixes and improvements
by peter＠adpm.de 01 Sep '15

01 Sep '15

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(a)adpm.de

1 0

Re: (ITS#8230) [PATCH] totp: bug fixes and improvements
by hyc＠symas.com 31 Aug '15

31 Aug '15

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/

1 0

Re: (ITS#8230) [PATCH] totp: bug fixes and improvements
by peter＠adpm.de 31 Aug '15

31 Aug '15

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(a)adpm.de

1 0

Re: (ITS#8230) [PATCH] totp: bug fixes and improvements
by h.b.furuseth＠usit.uio.no 30 Aug '15

30 Aug '15

On 29/08/15 17:44, hyc(a)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).

1 0

(ITS#8232) potential crash from syncprov_op_abandon
by hyc＠openldap.org 30 Aug '15

30 Aug '15

Full_Name: Howard Chu Version: 2.4 OS: URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (73.15.219.210) Submitted by: hyc If a connection closes while syncprov_search_response is turning the current search op into a detached psearch op, connection_abandon() may get run on the connection while both the original and the detached copy of the op are present on the connection queue. detach_op will append the new copy onto the end of the connection's queue. connection_abandon will find the original op first, and call into syncprov_op_abandon. syncprov_op_abandon will cause the detached copy to be freed from the si->si_ops list (since both copies have same connid and msgid). If there has been no other activity on the connection, then the original op's o_next will be pointing to the detached copy. connection_abandon is using this o_next to iterate thru the connection's queue. After syncprov frees this copy, connection_abandon will probably SEGV. The fix is to prevent connection_abandon from calling abandon handlers on an op that has already been abandoned.

1 0

regression from ITS#8042
by hyc＠openldap.org 30 Aug '15

30 Aug '15

Full_Name: Howard Chu Version: 2.4.41 OS: URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (73.15.219.210) Submitted by: hyc presentlist_find() can SEGV if called when si->si_presentlist is NULL.

1 0

Re: (ITS#8230) [PATCH] totp: bug fixes and improvements
by hyc＠symas.com 30 Aug '15

30 Aug '15

peter(a)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/d67bffc4a361cecfce69fb4d14edb33… > 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/435976d4f2468946bd0c5081ce7e2ae… > 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/04c15b7b1e44d4d3167577702a8b77c… > 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/04c15b7b1e44d4d3167577702a8b77c… > 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(a)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/

1 0

(ITS#8230) [PATCH] totp: bug fixes and improvements
by peter＠adpm.de 30 Aug '15

30 Aug '15

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/d67bffc4a361cecfce69fb4d14edb33… contrib/passwd/totp: flexibilize decoding key In function totp_b32_pton() - allow lowercase characters in encoded string too - allow padding to be omitted (totally, not only parts) 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/435976d4f2468946bd0c5081ce7e2ae… contrib/passwd/totp: fix the big-endian case For the big-endian case, 'msg' wasn't set from 'tval' in generate(). * https://github.com/marschap/openldap/commit/04c15b7b1e44d4d3167577702a8b77c… 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! * https://github.com/marschap/openldap/commit/04c15b7b1e44d4d3167577702a8b77c… contrib/passwd/totp: support compiling using nettle 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(a)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

1 0

Re: (ITS#8229) ldapsearch: report failed search results to stderr even when ldif == 0
by ryan＠nardis.ca 29 Aug '15

29 Aug '15

On Fri, Aug 28, 2015 at 11:23:50PM +0200, Hallvard Breien Furuseth wrote: >We should be wary of changing ldapsearch output, since >people parse it in scripts. Good point. Adding a stderr message to the existing stdout message would safer. Still not as safe as no change, of course. >I do find the output quirky and would have liked it different. >Maybe yet another option could clean it up, or an -o flag similar >to ldif-wrap. Yet another option... :)

1 0

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

openldap-bugs August 2015