Theldus / wsServer

wsServer - a tiny WebSocket server library written in C
https://theldus.github.io/wsServer
GNU General Public License v3.0
427 stars 86 forks source link

runtime error for left shifts with fsanitize=address,undefined in sha1.c #74

Closed darudev closed 1 year ago

darudev commented 1 year ago

Hi and thank you for a very good websocket library in C.

I not really sure if this is legit as I Am not very skilled with C.

Anyway, got this when running an executable compiled with sanitize enabled: wsServer/sha1.c:250:46: runtime error: left shift of 128 by 24 places cannot be represented in type 'int'

My research suggests it may be fixed by doing a cast to uint32_t before shifting in SHA1ProcessMessageBlock function

Orignal:

    {   
        W[t] = context->Message_Block[t * 4] << 24; 
        W[t] |= context->Message_Block[t * 4 + 1] << 16; 
        W[t] |= context->Message_Block[t * 4 + 2] << 8;
        W[t] |= context->Message_Block[t * 4 + 3]; 
    }   

New:

    {   
        W[t] = (uint32_t)context->Message_Block[t * 4] << 24; 
        W[t] |= (uint32_t)context->Message_Block[t * 4 + 1] << 16; 
        W[t] |= (uint32_t)context->Message_Block[t * 4 + 2] << 8;
        W[t] |= (uint32_t)context->Message_Block[t * 4 + 3]; 
    }   

But I don't know if will cause issues in some other parts of the library or whatever?

Theldus commented 1 year ago

Hi @darudev, Thanks for observing this... this error does really happen....

It turns out that this sha1.c code was not written by me, and I put it without modifications on wsServer... so it really may contain hidden errors that I don't know exactly where they are.

Looking at similar implementations of sha1, I saw that many of them make a cast at the same place where you suggested... so I don't think it would hurt the code, although I'm unable to tell it with 100% of certainty.

Similar code: a) gpac/gpac/src/utils/sha1.c b) MonetDB/MonetDB/common/utils/sha.c c) CyanogenMod/android_device_samsung_klte-common/fingerprint/hash.c

If you want, you can send a PR with this fix, or I'll add it myself =).

darudev commented 1 year ago

Hi, and sorry for late response! Probably easier if you just add it yourself?

Theldus commented 1 year ago

@darudev Sure, no problem. Commit 476a674 fixes this, thanks for reporting =).