shell/rtc: Fix out of bounds access; document error behavior by chrysn · Pull Request #19141 · RIOT-OS/RIOT

chrysn · 2023-01-13T15:09:01Z

Contribution description

When converting a date passed through the shell, the day-of-week calculation performs an out of bounds read access to the month-to-day-of-week calculation array by entering a month outside the 1-12 range. Especially on devices with bit-banded memory, this allows a shell user to read arbitrary memory.

Security Impact

The issue's impact is considered low because the RIOT shell is not generally accessible to users who are not the owner of the device.

Testing procedure

Should be trivial enough, especially as the difference is hard to spot interactively.

On native, run the default example (and wait for the traffic to settle).

Then, run

> rtc poweron
> rtc settime 2022-01-01 00:00:00
> rtc settime 2022-99-01 00:00:00

Both still work, but the latter doesn't access unassigned memory any more

Issues/PRs references

This fixes an issue that was submitted via the security mailing list by Guy Farrelly.

[edit: added missing description, added name of reporter]

maribu

bors merge

bors · 2023-01-13T15:24:55Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

miri64 · 2023-01-13T15:51:32Z

sys/shell/cmds/rtc.c

@@ -39,10 +40,24 @@ static int dow(int year, int month, int day)
 {
    /* calculate the day of week using Tøndering's algorithm */
    static int t[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};


Unrelated optimization proposal: this could be

Suggested change

static int t[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};

static const int8_t t[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};

(or even uint8_t but that's less about optimization and more type correctness)

Why is this even needed?
rtc_tm_normalize() will already do this - no need to implement this logic in a shell command.

(Ok, wday and yday are only set with RTC_NORMALIZE_COMPAT enabled because no RTC implementation uses those - but that's even less reason to have them calculated here)

Hm, indeed rtc_utils.c's _wday is a duplicate of this function, with different properties (it already has the const uint8_t type; it doesn't perform overflow checks but then again is only called with pre-sanitized input).

Honestly, if it was for me, struct tm would have no place in an embedded OS at all -- I won't impede work on it, and am happy to handle security issues arising from it (even biting down any snark), but addressing this duplication will require judgement calls on "Should the shell RTC command do the full normalization rtc_tm_normalize performs?" and "Should the shell RTC command pull in RTC_NORMALIZE_COMPAT=1?", and my answer to both would be mu.

riot-ci · 2023-01-13T17:23:34Z

Murdock results

✔️ PASSED

cd62aea shell/rtc: Fix out of bounds access; document error behavior

Success	Failures	Total	Runtime
6768	0	6768	12m:19s

Artifacts

Documentation preview

maribu · 2023-01-13T17:39:59Z

bors merge

bors · 2023-01-13T20:03:52Z

Build succeeded:

19143: boards: common: stdio_cdc_acm: let tests wait a bit for serial port [backport 2023.01] r=kaspar030 a=kaspar030 # Backport of #19128 19144: shell/rtc: Fix out of bounds access; document error behavior [backport 2023.01] r=kaspar030 a=kaspar030 # Backport of #19141 ### Contribution description ### Testing procedure Should be trivial enough, especially as the difference is hard to spot interactively. On native, run the default example (and wait for the traffic to settle). Then, run ``` > rtc poweron > rtc settime 2022-01-01 00:00:00 > rtc settime 2022-99-01 00:00:00 ``` Both still work, but the latter doesn't access unassigned memory any more ### Issues/PRs references This fixes an issue that was submitted anonymously. Co-authored-by: Martine Lenders <[email protected]> Co-authored-by: chrysn <[email protected]>

19144: shell/rtc: Fix out of bounds access; document error behavior [backport 2023.01] r=kaspar030 a=kaspar030 # Backport of #19141 ### Contribution description ### Testing procedure Should be trivial enough, especially as the difference is hard to spot interactively. On native, run the default example (and wait for the traffic to settle). Then, run ``` > rtc poweron > rtc settime 2022-01-01 00:00:00 > rtc settime 2022-99-01 00:00:00 ``` Both still work, but the latter doesn't access unassigned memory any more ### Issues/PRs references This fixes an issue that was submitted anonymously. Co-authored-by: chrysn <[email protected]>

19010: bootloaders/riotboot: add tinyUSB DFU support r=benpicco a=gschorcht ### Contribution description This PR provides - the tinyUSB DFU and DFU Runtime support and - the `riotboot_tinyusb_dfu` bootloader that uses the tinyUSB DFU mode to flash new application images. ~This PR includes PR #18983 for now to be compilable.~ ### Testing procedure 1. Use any board that supports the `riotboot´ and `tinyusb_device` features and flash the bootloader first, for example ``` BOARD=nucleo-f767zi make -C bootloaders/riotboot_tinyusb_dfu flash ``` and check that the `riotboot_tinyusb_dfu` bootloader is in DFU mode: ``` dfu-util --list ``` 3. Flash a first application using the following command: ``` FEATURES_REQUIRED=riotboot USEMODULE=tinyusb_dfu BOARD=nucleo-f767zi \ make -C tests/saul PROGRAMMER=dfu-util riotboot/flash-slot0 ``` and check that the application starts and is seen as upgradable: ``` dfu-util --list ``` 4. Restart the node in bootloader DFU mode by: ``` dfu-util -e ``` Flash a second application, for example ``` FEATURES_REQUIRED=riotboot USEMODULE=tinyusb_dfu BOARD=nucleo-f767zi \ make -C tests/shell PROGRAMMER=dfu-util riotboot/flash-slot1 ``` and check that the second application starts and is seen as upgradable: ``` dfu-util --list ``` ### Issues/PRs references ~Depends on PR #18983~ 19149: SECURITY: Describe that declassification is an option r=benpicco a=chrysn ### Contribution description Our security policy does not contain provisions for the case when what is reported is not what we consider an actual security issue. As it is described now, everything reported through security@ would go through the full treatment, including a point release. I'm not sure it belongs into the text itself (as it's more about how security reporters interact with the project than internals), but declassification should IMO be backed at least by 3 maintainers, and no strong NACK. ### Issues/PRs references #19141 followed that procedure after some chat on it on the maintainers channel. (In the discussion, I proposed declassification, with 2.5 people supporting it and one "I was about to, but can we be sure nobody is using it?" voice). Co-authored-by: Gunar Schorcht <[email protected]> Co-authored-by: chrysn <[email protected]>

chrysn added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jan 13, 2023

chrysn requested a review from kaspar030 January 13, 2023 15:09

github-actions bot added the Area: sys Area: System label Jan 13, 2023

shell/rtc: Fix out of bounds access; document error behavior

cd62aea

chrysn force-pushed the rtc-dow-overflow branch from fe52a4d to cd62aea Compare January 13, 2023 15:14

maribu approved these changes Jan 13, 2023

View reviewed changes

maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: security Area: Security-related libraries and subsystems labels Jan 13, 2023

miri64 reviewed Jan 13, 2023

View reviewed changes

kaspar030 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 13, 2023

bors bot merged commit d89379e into RIOT-OS:master Jan 13, 2023

kaspar030 mentioned this pull request Jan 13, 2023

shell/rtc: Fix out of bounds access; document error behavior [backport 2023.01] #19144

Merged

chrysn deleted the rtc-dow-overflow branch January 15, 2023 10:05

chrysn mentioned this pull request Jan 15, 2023

SECURITY: Describe that declassification is an option #19149

Merged

benpicco mentioned this pull request Feb 7, 2023

shell/rtc: use rtc_tm_normalize() to sanitize input #19212

Merged

jia200x added this to the Release 2023.04 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shell/rtc: Fix out of bounds access; document error behavior#19141

shell/rtc: Fix out of bounds access; document error behavior#19141
bors[bot] merged 1 commit intoRIOT-OS:masterfrom
chrysn-pull-requests:rtc-dow-overflow

chrysn commented Jan 13, 2023 •

edited

Loading

Uh oh!

maribu left a comment

Uh oh!

bors bot commented Jan 13, 2023

Uh oh!

miri64 Jan 13, 2023

Uh oh!

miri64 Jan 13, 2023

Uh oh!

benpicco Jan 13, 2023 •

edited

Loading

Uh oh!

chrysn Jan 15, 2023

Uh oh!

riot-ci commented Jan 13, 2023

Uh oh!

maribu commented Jan 13, 2023

Uh oh!

bors bot commented Jan 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

	static int t[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};
	static const int8_t t[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};

Conversation

chrysn commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Security Impact

Testing procedure

Issues/PRs references

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Jan 13, 2023

Uh oh!

miri64 Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

benpicco Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrysn Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

riot-ci commented Jan 13, 2023

Murdock results

Artifacts

Uh oh!

maribu commented Jan 13, 2023

Uh oh!

bors bot commented Jan 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

chrysn commented Jan 13, 2023 •

edited

Loading

benpicco Jan 13, 2023 •

edited

Loading