Fw: sha1 linux/mppe fix for 64-bit Linux

Fw: sha1 linux/mppe fix for 64-bit Linux

[Guy Rouillier]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

This is one of two messages addressing a fix to the sha1 code in PPP to
enable it to work on 64-bit systems.  The fix addressed in the current
message is for the linux/mppe component; the corresponding fix in the
other message is for the pppd component.  Both sets of changes are
required.  Without either I was encountering a segfault.  With only the
pppd changes, I got a kernel panic.  With both sets of changes, I am
able to use pptp with ppp to connect with a Microsoft server.

The major set of changes was to convert long data types to data types
with explicit length.  Basically, I changed longs to 32-bit data values
and unsigned chars to 8-bit values.  I also included a wrapper sha1()
function that I found in later versions of the code by the author, Steve
Reid.

This is my first submittal, so I apologize if I am not submitting this
properly.  I followed the advice I was given and read the submittal
guidelines at gnu.org.  However, after running diff -u against the ppp
component I pulled out of cvs and against my updates, I wasn't able to
run patch successfully against the result.  I asked on the
help.gnu.utils newsgroup but received no reply and I'm going out of town
for the rest of the week.  Because of this, I'm including both the diff
and my modified sources in the attached file.  I will gladly redo this
when I return if there is a better way to do it.

-- 
Guy Rouillier

[-- Attachment #2: linux.tar.gz --]
[-- Type: application/octet-stream, Size: 3370 bytes --]

Re: Fw: sha1 linux/mppe fix for 64-bit Linux

[Oleg Makarenko]

[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]

Hi Guy,

Guy Rouillier wrote:

>The major set of changes was to convert long data types to data types
>with explicit length.  Basically, I changed longs to 32-bit data values
>and unsigned chars to 8-bit values.  
>

Do you really need to change unsigned char to u8? Does algorithm break 
without that change? Or are to trying to preform some optimization?

While it doesn't hirt to change unsigned char to u8 and unsigned 
int/long to u32 it only makes sense when the int/long size does matter.

int32_t i;

is not a good replacement for plain

int i;

it only makes reader wonder why in hell it does matter if i is only used 
in for (i = 0; i < 8; i++)

Agree?

For the  sha1.c code the only 4-bit unclean place is a macro definition 
for rol, blk0 and hence R0, R1 etc . These macros all expect 32-bits 
arguments (as you have found in your previous mails)

That is why the only needed change to make it work on AMD64 (and 
hopefully not to break it on other platforms) is

 SHA1_Transform(unsigned long state[5], const unsigned char buffer[64])
 {
-    unsigned long a, b, c, d, e;
+    u_int32_t a, b, c, d, e;
     typedef union {
        unsigned char c[64];
-       unsigned long l[16];
+       u_int32_t l[16];
     } CHAR64LONG16;
     CHAR64LONG16 *block;

(replace u_int32_t with u8 for linux code part).

That is the change (see attachment for the patch) I use for several days 
without any problems.

Why to change other (not broken) parts of code? Do you gain some 
speed/size/other benefits with your unsigned char->u8 changes?

>I also included a wrapper sha1()
>function that I found in later versions of the code by the author, Steve
>Reid.
>  
>

What is the point to define a new function sha1() that is never used in 
the code? Probably something else is missed from your patch?

It is better to read man etc for diff/patch usage but still here is a 
quick guide.

Try to get clean ppp code and save it to (say) ppp-cvs20040615/ 
directory, make the directory copy to something like ppp.orig/, make all 
your changes in the ppp-cvs20040615/ directory (for both linux/sha1.* 
and pppd/sha1.*  files) and then run

diff -urN ppp.orig ppp-cvs20040615 > guy-x86_64.patch

Hope that helps

And in case you've missed it there is a patch on pptp-devel list to get 
rid of sha1.c in kernel code. This patch doesn't work for me so I still 
use a trivial fix to sha1.c but it seems the way to go. Did you try it?

regards,
Oleg


[-- Attachment #2: pppd-x86_64.patch --]
[-- Type: text/plain, Size: 1463 bytes --]

diff -urN ppp-2.4.2_cvs_20030610.orig/linux/mppe/sha1.c ppp-2.4.2_cvs_20030610/linux/mppe/sha1.c
--- ppp-2.4.2_cvs_20030610.orig/linux/mppe/sha1.c	2002-04-02 18:01:37.000000000 +0400
+++ ppp-2.4.2_cvs_20030610/linux/mppe/sha1.c	2004-06-08 14:55:46.000000000 +0400
@@ -19,6 +19,7 @@
 #if defined(__linux__)
 #include <asm/byteorder.h>
 #include <linux/string.h>
+#include <linux/types.h>
 #else if defined(__solaris__)
 #include <sys/isa_defs.h>
 #include <sys/ddi.h>
@@ -59,10 +60,10 @@
 static void
 SHA1_Transform(unsigned long state[5], const unsigned char buffer[64])
 {
-    unsigned long a, b, c, d, e;
+    u32 a, b, c, d, e;
     typedef union {
 	unsigned char c[64];
-	unsigned long l[16];
+	u32 l[16];
     } CHAR64LONG16;
     CHAR64LONG16 *block;
 
diff -urN ppp-2.4.2_cvs_20030610.orig/pppd/sha1.c ppp-2.4.2_cvs_20030610/pppd/sha1.c
--- ppp-2.4.2_cvs_20030610.orig/pppd/sha1.c	2002-04-02 17:54:59.000000000 +0400
+++ ppp-2.4.2_cvs_20030610/pppd/sha1.c	2004-06-08 14:54:44.000000000 +0400
@@ -18,6 +18,7 @@
 
 #include <string.h>
 #include <netinet/in.h>	/* htonl() */
+#include <sys/types.h>	/* u_int32_t */
 #include "sha1.h"
 
 static void
@@ -44,10 +45,10 @@
 static void
 SHA1_Transform(unsigned long state[5], const unsigned char buffer[64])
 {
-    unsigned long a, b, c, d, e;
+    u_int32_t a, b, c, d, e;
     typedef union {
 	unsigned char c[64];
-	unsigned long l[16];
+	u_int32_t l[16];
     } CHAR64LONG16;
     CHAR64LONG16 *block;
 

Re: sha1 linux/mppe fix for 64-bit Linux

[Guy Rouillier]

Oleg, thanks for the reply.  I've been out of town all week and just
received your messages.  Responses inline.

On Wed, 16 Jun 2004 00:00:42 +0400
Oleg Makarenko <mole@quadra.ru> wrote:

> Hi Guy,
> 
> Guy Rouillier wrote:
> 
> >The major set of changes was to convert long data types to data types
> >with explicit length.  Basically, I changed longs to 32-bit data
> >values and unsigned chars to 8-bit values.  
> >
> 
> Do you really need to change unsigned char to u8? Does algorithm break

My reason for doing this was three-fold.  (1) In the later version of
this code I located from the original author, he had changed it like
that.  As long as I was going through all the code anyway, I figured I
would follow his example and make every data type explicit.  As you say,
it doesn't hurt anything.  (2) I wanted to make the kernel part and the
pppd part of the code parallel.  Because I made them all explicit in
one, I made them all explicit in the other.   Ease of maintenance.  (3)
I wanted to do everything in one pass.  Because it required a kernel
rebuild, I didn't want to do just some minimal part I thought would
work, only to find out it still generated a segfault or kernel panic. 
Then I'd have to make additional changes and rebuild the kernel again.

> 
> without that change? Or are to trying to preform some optimization?
> 
> While it doesn't hirt to change unsigned char to u8 and unsigned 
> int/long to u32 it only makes sense when the int/long size does
> matter.
> 
> int32_t i;
> 
> is not a good replacement for plain
> 
> int i;
> 
> it only makes reader wonder why in hell it does matter if i is only
> used in for (i = 0; i < 8; i++)
> 
> Agree?

For counters I agree.  But int is again an ambiguous data type.   Who
knows what subsequent changes may occur that cause incompatibilities
with a 32-bit integer?  I probably did go overboard, but it makes the
code more bulletproof.

> 
> For the  sha1.c code the only 4-bit unclean place is a macro
> definition for rol, blk0 and hence R0, R1 etc . These macros all
> expect 32-bits arguments (as you have found in your previous mails)
> 
> That is why the only needed change to make it work on AMD64 (and 
> hopefully not to break it on other platforms) is
> 
>  SHA1_Transform(unsigned long state[5], const unsigned char
>  buffer[64]){
> -    unsigned long a, b, c, d, e;
> +    u_int32_t a, b, c, d, e;
>      typedef union {
>         unsigned char c[64];
> -       unsigned long l[16];
> +       u_int32_t l[16];
>      } CHAR64LONG16;
>      CHAR64LONG16 *block;
> 
> (replace u_int32_t with u8 for linux code part).

I don't think this is sufficient. a,b,c,d,e are assigned values from
state[5], so those two data types should agree.  To use your example, a
subsequent programmer may wonder why you are using type-safe variables
but ambiguous parameters in the same function.

> 
> That is the change (see attachment for the patch) I use for several
> days without any problems.

That may be, but it is still ambiguous.

> 
> Why to change other (not broken) parts of code? Do you gain some 
> speed/size/other benefits with your unsigned char->u8 changes?

Again, consistency, ease of maintenance, eliminate ambiguity, respect
the original author.

> 
> >I also included a wrapper sha1()
> >function that I found in later versions of the code by the author,
> >Steve Reid.
> >  
> >
> 
> What is the point to define a new function sha1() that is never used
> in the code? Probably something else is missed from your patch?

Because the original author has introduced it, and the ppp authors may
choose to replace separate invocations of SHA1_Init(), SHA1_Update(),
SHA1_Final() with a single sha1() call.  Simple code/time saver where it
can be used.

> 
> It is better to read man etc for diff/patch usage but still here is a 
> quick guide.

Tried that, didn't help.  Tried asking for help, got none.

> 
> Try to get clean ppp code and save it to (say) ppp-cvs20040615/ 
> directory, make the directory copy to something like ppp.orig/, make
> all your changes in the ppp-cvs20040615/ directory (for both
> linux/sha1.* and pppd/sha1.*  files) and then run
> 
> diff -urN ppp.orig ppp-cvs20040615 > guy-x86_64.patch

Thanks, that's essentially what I did.

> 
> Hope that helps
> 
> And in case you've missed it there is a patch on pptp-devel list to
> get rid of sha1.c in kernel code. This patch doesn't work for me so I
> still use a trivial fix to sha1.c but it seems the way to go. Did you
> try it?

No, because I knew changes were required in two places, and others had
already said the patch didn't work for them, I didn't try it.  I was
under time pressure to get it working.

One question for you if I may.  Do you know why multiple data type
systems have been introduced?  For example, the kernel code uses u32
while the pppd code uses u_int32_t.  Why not pick one and use it
everywhere?

> 
> regards,
> Oleg
> 
> 


-- 
Guy Rouillier


-- 
Guy Rouillier

Re: sha1 linux/mppe fix for 64-bit Linux

[Oleg Makarenko]

Hi Guy,

If you want to replace sha1.c with something more 64bit clean and feel 
that simple fix is not enough start from what seems to be the better 
(newer?) version at http://sea-to-sky.net/~sreid/sha1.c

As you can see from the above link the code was patched for 16bits and 
64bits machines.

You only need to change uint32 to posix's u_int32_t etc to use typedefs 
from system or ppp include files and get rid of main().

Then it could be dropped in instead of current sha1.c. Btw the uintNN 
changes are from Alpha guy.

>No, because I knew changes were required in two places, 
>
>and others had
>already said the patch didn't work for them, 
>
Probably it was only me :)

>I didn't try it.  I was
>under time pressure to get it working.
>  
>
The latest version works for me on both 32 and 64 bit kernels. The patch 
makes mppe code to use kernel's sha1 and arc4 implementations.

For user mode part we use something similar here and link pppd with 
libcrypto and change ms_chap.c with

#ifdef USE_OPENSSL_SHA1
#include <openssl/sha.h>
#define SHA1_SIGNATURE_SIZE SHA_DIGEST_LENGTH
#define SHA1_CTX SHA_CTX
#else
#include "sha1.h"
#endif

So we don't use ppp's sha1 implementation any more and that is why I 
don't care on how clean sha1.c is and think the simplest fix is enough.

>One question for you if I may.  Do you know why multiple data type
>systems have been introduced?  
>

No, I don't know. But I believe they are parts of different standards or 
conventions.

>For example, the kernel code uses u32
>while the pppd code uses u_int32_t.  Why not pick one and use it
>everywhere?
>  
>
and kernel also has __u32.  Others have u_int, uint, WORD, DWORD etc.

u32 and friends are linux specific, u_int32_t and alikes are from POSIX.

And "official" (C99) way is actually uint32_t. The "problematic" 
variables in sha1.c code could be declared as uint_least32_t in C99.

regards,
Oleg