Re: OpenSSL breaks factor(6)

> On Fri, Dec 27, 2019 at 07:00:04PM -0800, Rodney W. Grimes wrote:
> > > On Fri, Dec 27, 2019 at 01:47:17PM -0800, Steve Kargl wrote:
> > > > On Fri, Dec 27, 2019 at 01:25:30PM -0800, Steve Kargl wrote:
> > > > > The use of OpenSSL in factor(6) breaks factor(6) with respect to
> > > > > its documentation.
> > > > > 
> > > > > % man factor
> > > > >   ...
> > > > >   Numbers may be preceded by a single '+'.
> > > > >   ...
> > > > > 
> > > > > % factor +125
> > > > > factor: +125: illegal numeric format.
> > > > > 
> > > > 
> > > > This fixes factor(6) for the above issue.  The issue with
> > > > hexadecimal is not easily fixed.
> > > >
> > >  
> > > This patch now includes a fix for hexadecimal conversion.  It
> > > simple scans the string for a hex digit in [a,...,f] and assumes
> > > that a hexadecimal string has been entered.  A string that includes
> > > character from the decimal digits is assumed to by a decimal
> > > representation.  
> > 
> > It looks to me that the old code did the common method of
> > try to convert as decimal, if that fails, try it as hex,
> > if that fails report an error.
> > 
> > Why is is that this common logic no longer works?
> 
> AFAICT, BN_dec2bn and BN_hex2bn from OpenSSL scan from left
> to right, does a conversion with what is possible, and reports
> success.  That is, for 1abc, BN_dec2bn can convert 1 to 1 and
> reports success.  The local implementations of these functions,
> when OpenSSL is not used, does not do this partial conversion.

I think I see now, the local implementaton checks for whole
string conversion with a test for newline or null as the last
byte converted by strtoul, the OpenSSL does not do this.

So why ever use the, um, IMHO broken for this application,
SSL versions of these functions?  Or if we do need to use
them for some reason apply the whole string conversion
checks as wrappers around them?

> > > 
> > > Index: factor.c
> > > ===================================================================
> > > --- factor.c	(revision 355983)
> > > +++ factor.c	(working copy)
> > > _at__at_ -71,6 +71,7 _at__at_
> > >  #include <errno.h>
> > >  #include <inttypes.h>
> > >  #include <limits.h>
> > > +#include <stdbool.h>
> > >  #include <stdio.h>
> > >  #include <stdlib.h>
> > >  #include <unistd.h>
> > > _at__at_ -104,6 +105,7 _at__at_
> > >  
> > >  #endif
> > >  
> > > +static bool	is_hex(char *str);
> > >  static void	BN_print_dec_fp(FILE *, const BIGNUM *);
> > >  
> > >  static void	pr_fact(BIGNUM *);	/* print factors of a value */
> > > _at__at_ -148,21 +150,25 _at__at_
> > >  			for (p = buf; isblank(*p); ++p);
> > >  			if (*p == '\n' || *p == '\0')
> > >  				continue;
> > > +			if (*p == '+') p++;
> > >  			if (*p == '-')
> > >  				errx(1, "negative numbers aren't permitted.");
> > > -			if (BN_dec2bn(&val, buf) == 0 &&
> > > -			    BN_hex2bn(&val, buf) == 0)
> > 
> > Why does this logic fail?
> 
> See BN_hex2bn manpage.   C/C++ does shortcircuits.  With 1abc,
> BN_dec2bn converts the string to 1, puts it in val, and returns
> nonzero.  BN_hex2bn is never called.  Flipping the conditionals,
> of course, doesn't work because 0-9 are digits in the hexadecimal
> set (e.g., 111 is a valid hex and decimal string).
> 
> > > -				errx(1, "%s: illegal numeric format.", buf);
> > > +			ch = is_hex(p) ? BN_hex2bn(&val, p) :
> > > +			    BN_dec2bn(&val, p);
> > > +			if (ch == 0)
> > > +				errx(1, "%s: illegal numeric format.", p);
> > >  			pr_fact(val);
> > >  		}
> > >  	/* Factor the arguments. */
> > >  	else
> > > -		for (; *argv != NULL; ++argv) {
> > > -			if (argv[0][0] == '-')
> > > +		for (p = *argv; p != NULL; p = *++argv) {
> > > +			if (*p == '-')
> > >  				errx(1, "negative numbers aren't permitted.");
> > > -			if (BN_dec2bn(&val, argv[0]) == 0 &&
> > > -			    BN_hex2bn(&val, argv[0]) == 0)
> > > -				errx(1, "%s: illegal numeric format.", argv[0]);
> > > +			if (*p == '+') p++;
> > > +			ch = is_hex(p) ? BN_hex2bn(&val, p) :
> > > +			    BN_dec2bn(&val, p);
> > > +			if (ch == 0)
> > > +				errx(1, "%s: illegal numeric format.", p);
> > >  			pr_fact(val);
> > >  		}
> > >  	exit(0);
> > > _at__at_ -343,10 +349,9 _at__at_
> > >  BN_dec2bn(BIGNUM **a, const char *str)
> > >  {
> > >  	char *p;
> > > -
> > This blank line is part of style(9)
> > 
> 
> Whoops.  Haven't had to worry about style(9) in a long time.
> 
> > >  	errno = 0;
> > >  	**a = strtoul(str, &p, 10);
> > > -	return (errno == 0 && (*p == '\n' || *p == '\0'));
> > > +	return (errno == 0 ? 1 : 0);	/* OpenSSL returns 0 on error! */
> > >  }
> > >  
> > >  static int
> > > _at__at_ -356,7 +361,7 _at__at_
> > >  
> > >  	errno = 0;
> > >  	**a = strtoul(str, &p, 16);
> > > -	return (errno == 0 && (*p == '\n' || *p == '\0'));
> > > +	return (errno == 0 ? 1 : 0);	/* OpenSSL returns 0 on error! */
> > >  }
> > >  
> > >  static BN_ULONG
> > > _at__at_ -370,3 +375,17 _at__at_
> > >  }
> > >  
> > >  #endif
> > > +
> > > +/* Check if the string contains a hexadecimal digit. */
> > > +static bool
> > > +is_hex(char *str)
> > This function is poorly named as it does not check for
> > all valid hex digits, only for alpha hex digits.  It
> > also only accepts lower case hex alpha, I would expect
> > hex input to be case insensitive.
> > 
> > is_hexalpha?
> 
> Feel free to rename it and improve.
> 
> > 
> > > +{
> > > +	char c, *p;
> > > +	for (p = str; *p; p++) {
> > > +		c = tolower(*p);
> > > +		if (c == 'a' || c == 'b' || c == 'c' || c == 'd' ||
> > > +		    c == 'e' || c == 'f')
> > 		if ( c >= 'a' || c <= 'f')
> > 
> > > +			return true;
> > > +	}
> > > +	return false;
> > > +}
> > > 
> > > -- 
> > > Steve
> > 
> > -- 
> > Rod Grimes                                                 rgrimes_at_freebsd.org
> 
> -- 
> Steve
> 20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
> 20161221 https://www.youtube.com/watch?v=IbCHE-hONow
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"
> 

-- 
Rod Grimes                                                 rgrimes_at_freebsd.org