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

[hyc@symas.com]

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/