search

InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.71k stars 927 forks source link

Fix potential buffer overflows when calling sprintf #1742

Closed szsam closed 10 months ago

github-actions[bot] commented 1 year ago
Build size and comparison to main: Section Size Difference
text 377632B 16B
data 940B 0B
bss 63492B 0B
minacode commented 1 year ago

Welcome to InfiniTime! 😊 I think it would be helpful if you write a little description about the motivations of the changes you made.

Avamander commented 1 year ago

These aren't overflows, truncations at best and modulo is relatively quite expensive.

The other parts, would be very nice if you would explain them.

szsam commented 1 year ago

A recent commit introduces a real one-byte buffer overflow. https://github.com/InfiniTimeOrg/InfiniTime/blob/40f7e1c7be6882e01058b5ccf64d5005c6105346/src/displayapp/screens/StopWatch.cpp#L201-L206 The format str "#%2d %2d:%02d:%02d.%02d\n" requires 17 bytes, but the buffer length is only 16. This is fixed in this PR.

szsam commented 1 year ago

I don't think that replacing sprintf with snprintf is necessarily a good idea. I believe that because it adds an overhead and that, with the current configuration, I don't think it does anything if it detects an overflow, we shouldn't use it.

I agree that snprintf adds overhead. However, when snprintf(buffer, bufsz, fmt, ...) detects an overflow, it will write at most bufsz-1 characters, effectively preventing buffer overflow.

That said, if you insist on using sprintf, I will change the code and use it.

szsam commented 10 months ago

@FintasticMan Rebase done.

Machiry commented 6 months ago

Hello @FintasticMan, @Avamander, @NeroBurner ,

My name is Aravind Machiry, Assistant Professor at Purdue's ECE Department.

Thank you for considering this pull request. This pull request was the result of our on-going research work (along with @szsam) to improve the security and quality of open-source embedded projects.

In addition to scanning codebases with CodeQL, we are also doing a short (~4 minutes) survey to understand the use of static analysis tools like gcc -Wall and CodeQL in embedded software projects.

It would greatly benefit our research if you could fill this anonymous survey: https://purdue.ca1.qualtrics.com/jfe/form/SV_0OnXfr5plPe1QCa

Thank you, Aravind