Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integer overflow in icmpv6 echo packet creation #19829

Closed
mchesser opened this issue Jul 14, 2023 · 2 comments
Closed

Integer overflow in icmpv6 echo packet creation #19829

mchesser opened this issue Jul 14, 2023 · 2 comments

Comments

@mchesser
Copy link

Description

It is possible for an integer overflow of len/data_len to occur as part of gnrc_icmpv6_echo_send/gnrc_icmpv6_echo_build when computing the total length of the packet including the header i.e. data_len + sizeof(imcpv6_echo_t) at gnrc_icmpv6_echo.c:34

Depending on the value used this results in either either a null pointer dereference at gnrc_icmpv6.c:136 (if len == (SIZE_MAX - 8)) or a buffer overflow at gnrc_icmpv6_echo.c:181 (if (SIZE_MAX - 8) < len <= SIZE_MAX).

It's probably more an issue of API misuse than anything -- but it is pretty easy to accidentally trigger through the shell interface (see below):

Steps to reproduce the issue

It’s pretty easy to trigger in the gnrc_networking example because the -s options allows negative numbers. Meaning we can run the following to get a segfault:

main(): This is RIOT! (Version: 2023.07-devel-693-g561e1)
RIOT network stack example application
All up, running the shell now
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault (core dumped)
bors bot added a commit that referenced this issue Sep 19, 2023
19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
bors bot added a commit that referenced this issue Sep 19, 2023
19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
bors bot added a commit that referenced this issue Sep 19, 2023
19923: boards: add Silabs EFM32 Giant Gecko GG11 Starter Kit r=miri64 a=gschorcht

### Contribution description

The PR adds the support for the EFM32GG11B family and the Silabs EFM32 Giant Gecko GG11 Starter Kit board.

The Silabs EFM32 Giant Gecko GG11 has the following on-board features:

- EFM32GG11B MCU with 2 MB flash and 512 kB RAM
- J-Link USB debugger
- 176x176 RGB LCD (not supported)
- 2 user buttons, 2 user RGB LEDs and a touch slider
- Si7021 Relative Humidity and Temperature Sensor
- Si7210 Hall-Effect Sensor (not supported)
- USB OTG interface (Device mode supported)
- 32 MByte Quad-SPI Flash (not supported yet)
- SD card slot (not supported yet, follow-up PR based on PR #19760)
- RJ-45 Ethernet (not supported)
- Dual microphones (not supported)

### Testing procedure

Basic tests should work.

### Issues/PRs references

19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
bors bot added a commit that referenced this issue Sep 19, 2023
19923: boards: add Silabs EFM32 Giant Gecko GG11 Starter Kit r=miri64 a=gschorcht

### Contribution description

The PR adds the support for the EFM32GG11B family and the Silabs EFM32 Giant Gecko GG11 Starter Kit board.

The Silabs EFM32 Giant Gecko GG11 has the following on-board features:

- EFM32GG11B MCU with 2 MB flash and 512 kB RAM
- J-Link USB debugger
- 176x176 RGB LCD (not supported)
- 2 user buttons, 2 user RGB LEDs and a touch slider
- Si7021 Relative Humidity and Temperature Sensor
- Si7210 Hall-Effect Sensor (not supported)
- USB OTG interface (Device mode supported)
- 32 MByte Quad-SPI Flash (not supported yet)
- SD card slot (not supported yet, follow-up PR based on PR #19760)
- RJ-45 Ethernet (not supported)
- Dual microphones (not supported)

### Testing procedure

Basic tests should work.

### Issues/PRs references

19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

19933: examples/gcoap: add saml11-xpro to CI boards with insufficient memory r=miri64 a=krzysztof-cabaj

### Contribution description

Bors run for PR #19927 reveals that ```examples\gcoap``` do not fit in saml11-xpro board. 
I checked and this same linker error also appears on current master branch. 

This PR add sampl11-xpro board to the Makefile.ci BOARD_INSUFFICIENT_MEMORY list.

### Testing procedure

See logs from bors https://ci.riot-os.org/details/073c5dadc6ba4bc8a613edb78a1a4a2d

### Issues/PRs references

PR #19927 


Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
bors bot added a commit that referenced this issue Sep 19, 2023
19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

19933: examples/gcoap: add saml11-xpro to CI boards with insufficient memory r=miri64 a=krzysztof-cabaj

### Contribution description

Bors run for PR #19927 reveals that ```examples\gcoap``` do not fit in saml11-xpro board. 
I checked and this same linker error also appears on current master branch. 

This PR add sampl11-xpro board to the Makefile.ci BOARD_INSUFFICIENT_MEMORY list.

### Testing procedure

See logs from bors https://ci.riot-os.org/details/073c5dadc6ba4bc8a613edb78a1a4a2d

### Issues/PRs references

PR #19927 


Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
bors bot added a commit that referenced this issue Sep 19, 2023
19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
bors bot added a commit that referenced this issue Sep 20, 2023
19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
bors bot added a commit that referenced this issue Sep 20, 2023
19927: sys/shell/ping: fix ping packet size overflow r=miri64 a=krzysztof-cabaj

### Contribution description

In #19829 `@mchesser` point out integer overflow in the ```ping``` command and API. This PR fix this issue in two ways:
1) Add protection in the API.
2) Add protection in the user command.

### Testing procedure

Without this PR passing negative number to the ```ping -s``` option cause segmentation fault, for example in the 
```example/gnrc_networking```:

```
> ping -s -7 ::1
ping -s -7 ::1
Segmentation fault
```

With this PR user shows appropriate warning test:

```
> ping -s -7 ::1
ping -s -7 ::1
ICMPv6 datagram size should be in range <0, 65527>.
> 
``` 

### Issues/PRs references

Issue #19829 

Co-authored-by: krzysztof-cabaj <kcabaj@gmail.com>
@krzysztof-cabaj
Copy link
Contributor

I think we could close that issue. Reported bug was fixed by PR #19927 which was successfully merged into RIOT master.

@miri64
Copy link
Member

miri64 commented Sep 27, 2023

Yepp, the trick to auto-close is to add „Fixes #“ to the OP or commit message btw. :-)

@miri64 miri64 closed this as completed Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants