search

kosma / minmea

a lightweight GPS NMEA 0183 parser library in pure C
Do What The F*ck You Want To Public License
735 stars 246 forks source link

Division by zero in minmea_tocoord #30

Closed gy741 closed 2 years ago

gy741 commented 6 years ago

Hello.

I found a divide by zero bug in minmea.

Please confirm.

Thanks.

OS: CentOS 7 64bit Version: commit cd27e72970cd50b46e9c95c1c0076a49c1ceaf33 PoC Download: FPE_minmea_tocoord.zip

ASAN:DEADLYSIGNAL
=================================================================
==15762==ERROR: AddressSanitizer: FPE on unknown address 0x000000513aa4 (pc 0x000000513aa4 bp 0x7ffefb6e80b0 sp 0x7ffefb6e80b0 T0)
    #0 0x513aa3 in minmea_tocoord /home/karas/minmea/minmea.h:250
    #1 0x513c60 in main /home/karas/minmea/example.c:35
    #2 0x7f92b2580c04 in __libc_start_main (/lib64/libc.so.6+0x21c04)
    #3 0x41ab5b in _start (/home/karas/minmea/example+0x41ab5b)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: FPE /home/karas/minmea/minmea.h:250 in minmea_tocoord
==15762==ABORTING

================= [Acknowledgement] This work was supported by ICT R&D program of MSIP/IITP. [R7518-16-1001, Innovation hub for high Performance Computing]

hraftery commented 6 years ago

Seems unlikely. What does your /home/karas/minmea/minmea.h file look like around line 250? It should be:

static inline float minmea_tocoord(struct minmea_float f) { if (f->scale == 0) return NAN; int_least32_t degrees = f->value / (f->scale 100); int_least32_t minutes = f->value % (f->scale 100); return (float) degrees + (float) minutes / (60 f->scale); }

which includes a divide by zero check. Would be an interesting test for a fuzzer to see if it can still cause an FPE even with the divide by zero check.

Heath

On 20 Feb 2018, at 10:32 am, gy741 notifications@github.com wrote:

Hello.

I found a divide by zero bug in minmea.

Please confirm.

Thanks.

OS: CentOS 7 64bit Version: commit cd27e72 https://github.com/kosma/minmea/commit/cd27e72970cd50b46e9c95c1c0076a49c1ceaf33 PoC Download: FPE_minmea_tocoord.zip https://github.com/kosma/minmea/files/1738501/FPE_minmea_tocoord.zip ASAN:DEADLYSIGNAL

==15762==ERROR: AddressSanitizer: FPE on unknown address 0x000000513aa4 (pc 0x000000513aa4 bp 0x7ffefb6e80b0 sp 0x7ffefb6e80b0 T0)

0 0x513aa3 in minmea_tocoord /home/karas/minmea/minmea.h:250

#1 0x513c60 in main /home/karas/minmea/example.c:35
#2 0x7f92b2580c04 in __libc_start_main (/lib64/libc.so.6+0x21c04)
#3 0x41ab5b in _start (/home/karas/minmea/example+0x41ab5b)

AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: FPE /home/karas/minmea/minmea.h:250 in minmea_tocoord ==15762==ABORTING

[Acknowledgement] This work was supported by ICT R&D program of MSIP/IITP. [R7518-16-1001, Innovation hub for high Performance Computing]

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kosma/minmea/issues/30, or mute the thread https://github.com/notifications/unsubscribe-auth/AFRsb59BZtdyOfbzhDpiOn5ou-_WzhRFks5tWgSPgaJpZM4SLOgY.

gy741 commented 6 years ago

@hraftery

Hello,

I have not changed the minmea.h file.

So the contents of the file are the same.

I did some analysis.

In my opinion, if the scale value negative, can pass the condition.

Thanks.

/home/karas/minmea/minmea.h:250

static inline float minmea_tocoord(struct minmea_float *f)
{
    if (f->scale == 0)
        return NAN;
    int_least32_t degrees = f->value / (f->scale * 100);
    int_least32_t minutes = f->value % (f->scale * 100);
    return (float) degrees + (float) minutes / (60 * f->scale);
}

GDB Log

Program received signal SIGFPE, Arithmetic exception.
0x00000000004009f8 in minmea_tocoord (f=0x7fffffffe2b4) at minmea.h:250
250         int_least32_t degrees = f->value / (f->scale * 100);
(gdb) p f->scale
$29 = -2147483648
(gdb) p  f->value / (f->scale * 100)
Division by zero
(gdb) p f->value
$20 = 0
(gdb) p f->scale * 100
$21 = 0

#0  0x00000000004009f8 in minmea_tocoord (f=0x7fffffffe2b4) at minmea.h:250
        degrees = 32767
        minutes = -7484
#1  0x0000000000400bb5 in main () at example.c:35
        frame = {time = {hours = -1, minutes = -1, seconds = -1, microseconds = -1}, valid = false, latitude = {value = 0, scale = -2147483648}, longitude = {value = 0, scale = 0}, speed = {value = 111,
            scale = 1000}, course = {value = 4, scale = 1}, date = {day = -1, month = -1, year = -1}, variation = {value = 0, scale = 0}}
        line = "$OSRMCGPNIT,,,.", '0' <repeats 21 times>, '1' <repeats 14 times>, ",,,,.111,4,,,,.11,,,.111,4,,,"

sample code

#include <stdio.h>
#include <stdint.h>

int main()
{
    int b = 1;
    int_least32_t scale = -2147483648;
        if (scale == 0){
             printf ("%d",b);
             return 0;
        }
    int_least32_t degrees = scale * 100;
    printf ("%d",degrees);
    return 0;
}
hraftery commented 6 years ago

Ah yes, f->scale is an integer. So overflow behaviour of f->scale * 100 is undefined (even if f->scale is positive). So yes, I agree, it looks like a bug.

I’m sure a pull request would be welcomed!

Heath

On 20 Feb 2018, at 1:49 pm, gy741 notifications@github.com wrote:

Hello,

I have not changed the minmea.h file.

So the contents of the file are the same.

I did some analysis.

In my opinion, if the scale value negative, can pass the condition.

Thanks.

static inline float minmea_tocoord(struct minmea_float f) { if (f->scale == 0) return NAN; int_least32_t degrees = f->value / (f->scale 100); int_least32_t minutes = f->value % (f->scale 100); return (float) degrees + (float) minutes / (60 f->scale); } GDB Log

Program received signal SIGFPE, Arithmetic exception. 0x00000000004009f8 in minmea_tocoord (f=0x7fffffffe2b4) at minmea.h:250 250 int_least32_t degrees = f->value / (f->scale 100); (gdb) p f->scale $29 = -2147483648 (gdb) p f->value / (f->scale 100) Division by zero (gdb) p f->value $20 = 0 (gdb) p f->scale * 100 $21 = 0

0 0x00000000004009f8 in minmea_tocoord (f=0x7fffffffe2b4) at minmea.h:250

    degrees = 32767
    minutes = -7484

1 0x0000000000400bb5 in main () at example.c:35

    frame = {time = {hours = -1, minutes = -1, seconds = -1, microseconds = -1}, valid = false, latitude = {value = 0, scale = -2147483648}, longitude = {value = 0, scale = 0}, speed = {value = 111,
        scale = 1000}, course = {value = 4, scale = 1}, date = {day = -1, month = -1, year = -1}, variation = {value = 0, scale = 0}}
    line = "$OSRMCGPNIT,,,.", '0' <repeats 21 times>, '1' <repeats 14 times>, ",,,,.111,4,,,,.11,,,.111,4,,,"

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kosma/minmea/issues/30#issuecomment-366852427, or mute the thread https://github.com/notifications/unsubscribe-auth/AFRsb_XJ87p3dCCwoKjcuL6wCcXRbyOcks5tWjKdgaJpZM4SLOgY.

gy741 commented 6 years ago

@hraftery

Hello,

I have sent a pull request.

Please let me know if there is a better patch method.

Pull : #31

hmm.... In my opinion, it is an insecure patch.

Thanks.

gy741 commented 6 years ago

@hraftery

Hello,

I modified the source code again.

What do you think of this source code?

Thanks.

static inline float minmea_tocoord(struct minmea_float *f)
{
    if (f->scale == 0)
        return NAN;
+     if (f->scale  > (INT_LEAST32_MAX / 100))
+       return NAN;
+     if (f->scale < (INT_LEAST32_MIN / 100))
+       return NAN;
      int_least32_t degrees = f->value / (f->scale * 100);
      int_least32_t minutes = f->value % (f->scale * 100);
      return (float) degrees + (float) minutes / (60 * f->scale);
  }
hraftery commented 6 years ago

Looks pretty good. I guess it’s now up to kosma to pull it!

On 22 Feb 2018, at 12:52 pm, gy741 notifications@github.com wrote:

@hraftery https://github.com/hraftery Hello,

I modified the source code again.

What do you think of this source?

Thanks.

static inline float minmea_tocoord(struct minmea_float *f) { if (f->scale == 0) return NAN;

gy741 commented 6 years ago

Hello,

thank you for confirmation.

I have sent a pull request.

Pull : #32

Thanks.

kosma commented 6 years ago

That's a good catch! I need to have another look at the code to see if there's some way to avoid the extra if-check by changing math. I should resolve this in a few days.

tobylo commented 4 years ago

@kosma I'm guessing you've dropped the ball on this one?

matonga commented 4 years ago

Should minmea_float.scale be an unsigned value?

cmorganBE commented 2 years ago

Sorry for the delay, merged the PR. @matonga can you reopen if there is still an issue and/or feel free to open a PR to resolve?