phc-discussions - Re: [PHC] Specification of a modular crypt format (2)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150929125548.GA15621@bolet.org>
Date: Tue, 29 Sep 2015 14:55:48 +0200
From: Thomas Pornin <pornin@...et.org>
To: discussions@...sword-hashing.net
Subject: Re: [PHC] Specification of a modular crypt format (2)

On Tue, Sep 29, 2015 at 03:05:59AM +0300, Alexander Cherepanov wrote:
> AIUI the decoder tries to be strict. Then perhaps it's worth to
> check that 'str' doesn't start with ' ' or '-' here and try to catch
> overflows.

Strictly speaking, the decoder tries to be strict when strictness is
cheap. If we want a strict decoder of numerical values, strtoul()
won't be enough; we would need something like this:

/* ------------------------------------------------------------------- */
#include <limits.h>

/*
 * Decode decimal integer from 'str'; the value is written in '*v'.
 * Returned value is a pointer to the next non-decimal character in the
 * string. If there is no digit at all, or the value encoding is not
 * minimal (extra leading zeros), or the value does not fit in an
 * 'unsigned long', then NULL is returned.
 */
static const char *
decode_decimal(const char *str, unsigned long *v)
{
	const char *orig;
	unsigned long acc;

	orig = str;
	acc = 0;
	for (orig = str;; str ++) {
		int c;

		c = *str;
		if (c < '0' || c > '9') {
			break;
		}
		c -= '0';
		if (acc > (ULONG_MAX / 10)) {
			return NULL;
		}
		acc *= 10;
		if ((unsigned long)c > (ULONG_MAX - acc)) {
			return NULL;
		}
		acc += (unsigned long)c;
	}
	if (str == orig || (*orig == '0' && str != (orig + 1))) {
		return NULL;
	}
	*v = acc;
	return str;
}
/* ------------------------------------------------------------------- */

(This code is completely untested; I just wrote it in this email.)

I am not sure this is worth the effort. It is not very complex to do
(less than 40 lines of C code).


> >	if (pp->m < 1 || (pp->m >> 30) > 3) {
> 
> Why not just compare with a constant here?

This is to avoid a spurious warning. If I write this:

	if (pp->m < 1 || pp->m > 0xFFFFFFFF)

then the compiler, on a 32-bit machine, will complain that the
comparison result is "always true". I don't want spurious warnings: they
train developers to ignore warnings.


> >	if (pp->p < 1 || pp->p > 64) {
> 
> 255 here?

Oh yes. Indeed. I fixed the specification, but forgot to change the code
accordingly. Thanks !


	--Thomas