Avoid crash on crash report when a bad function pointer was called #11298

MeirShpilraien · 2022-09-21T13:39:33Z

If Redis crashes due to calling an invalid function pointer, the backtrace function will try to de-reference this invalid pointer which will cause a crash inside the crash report and will kill the processes without having all the crash report information.

Example:

=== REDIS BUG REPORT START: Cut & paste starting from here ===
198672:M 19 Sep 2022 18:06:12.936 # Redis 255.255.255 crashed by signal: 11, si_code: 1
198672:M 19 Sep 2022 18:06:12.936 # Accessing address: 0x1
198672:M 19 Sep 2022 18:06:12.936 # Crashed running the instruction at: 0x1
// here the processes is crashing

This PR tries to fix this crash by:

Identify the issue when it happened.
Replace the invalid pointer with a pointer to some dummy function so that backtrace will not crash.

The identification is done by comparing eip to info->si_addr, if they are the same we know that the crash happened on the same address it tries to accesses and we can conclude that it tries to call and invalid function pointer.

To replace the invalid pointer we introduce a new function, setAndGetMcontextEip, which replaces getMcontextEip and allow to set the Eip for the different supported OS's. After printing the trace we retrieve the old Eip value.

If Redis crashes due to calling an invalid function pointer, the `backtrace` function will try to dereference this invalid pointer which will cause a crash inside the crash report and will kill the processes without having all the crash report infomation. Example: ``` === REDIS BUG REPORT START: Cut & paste starting from here === 198672:M 19 Sep 2022 18:06:12.936 # Redis 255.255.255 crashed by signal: 11, si_code: 1 198672:M 19 Sep 2022 18:06:12.936 # Accessing address: 0x1 198672:M 19 Sep 2022 18:06:12.936 # Crashed running the instruction at: 0x1 // here the processes is crashing ``` This PR tries to fix this crash be: 1. Identify the issue when it happened. 2. Replace the invalid pointer with a pointer to some dummy function so that `backtrace` will not crash. I identification is done by comparing `eip` to `info->si_addr`, if they are the same we know that the crash happened on the same address it tries to accesses and we can coclude that it tries to call and invalid function pointer. To replace the invalid pointer we introduce a new function, `setMcontextEip`, which is very similar to `getMcontextEip` and it knows to set the Eip for the different supported OS's. After printing the trace we retrieve the old `Eip` value.

oranagra · 2022-09-21T14:22:52Z

Full CI for this branch (passed):
https://github.com/redis/redis/actions/runs/3098579483/jobs/5016671554
https://github.com/redis/redis-extra-ci/actions/runs/3098590626

oranagra · 2022-09-21T14:23:42Z

@devnexen maybe you can test this on some other platforms?

devnexen · 2022-09-21T17:31:25Z

@devnexen maybe you can test this on some other platforms?

so tried
macos arm64 (seeing "eip" on non-x86 archs feels kind of weird :-) your PR just made me realised it), BSD and Illumos on x86_64. No issues so far.

src/debug.c

Co-authored-by: Oran Agra <oran@redislabs.com>

yossigo

@oranagra @MeirShpilraien The problem is clear, but don't you think this is going a bit too far into the C library's responsibility domain?

oranagra · 2022-09-24T18:53:41Z

@yossigo the C library is broken.
Are you suggesting we'll live with it?
If the fix is safe, I don't see any reason not to do it.

yossigo · 2022-09-28T17:21:54Z

@oranagra It's more platform-specific code that may have more undesired side-effect or extra maintenance burden in the long term, only to fix a non-critical libc issue for which there is a workaround (get a core dump). I do not feel so strongly about it, though. We can remove it if it becomes a problem.
We should probably open a glibc issue to have it fixed.

oranagra · 2022-09-29T05:56:37Z

@yossigo i feel the scope of this change is relatively small and we can cope with the side effects, or revert if we can't.
other than compilation errors, it can't really do any harm since it only runs in a very specific case.
i don't think the workaround of a core-dump is a good one since the crash report is needed in cases where the error happens in production and is not reproducible.

i tried filing a bug in glibc, as well as searching for an existing one (can't find it in their bugzilla), and it seems too complicated, and i'm short in time. (can't easily create an account, maybe someone who already has an account can try).
i do see however that this error was previously reported in their mailing list and ignored:
https://gcc.gnu.org/legacy-ml/gcc/2007-06/msg00329.html

iav20 · 2022-11-14T08:42:42Z

Hello Team ,

Can you please share the details by when the fix of this issue will be backported in 7.0 Release

Best Regards,
Apoorv

oranagra · 2022-11-14T09:01:57Z

@iav20 it'll be part of the next release we make for 7.0 and 6.2, but we didn't define the timeline yet.
unless some urgent issue is found that justifies a new release, it can take a few months.

esteinerMW · 2022-11-14T15:03:01Z

@oranagra Thank you for your feedback!

A data point regarding 6.2:
Porting the fix would address the following issue: (NIST NVD score was flagged by compliance scans)
https://nvd.nist.gov/vuln/detail/CVE-2022-3647

Thanks in advance for your consideration!
Eitan

yossigo · 2022-11-15T13:56:45Z

@esteinerMW I think this CVE is wrong, this is not really a denial of service issue. We'll see if this can be corrected, regardless of the fix being backported.

…11298) If Redis crashes due to calling an invalid function pointer, the `backtrace` function will try to dereference this invalid pointer which will cause a crash inside the crash report and will kill the processes without having all the crash report information. Example: ``` === REDIS BUG REPORT START: Cut & paste starting from here === 198672:M 19 Sep 2022 18:06:12.936 # Redis 255.255.255 crashed by signal: 11, si_code: 1 198672:M 19 Sep 2022 18:06:12.936 # Accessing address: 0x1 198672:M 19 Sep 2022 18:06:12.936 # Crashed running the instruction at: 0x1 // here the processes is crashing ``` This PR tries to fix this crash be: 1. Identify the issue when it happened. 2. Replace the invalid pointer with a pointer to some dummy function so that `backtrace` will not crash. I identification is done by comparing `eip` to `info->si_addr`, if they are the same we know that the crash happened on the same address it tries to accesses and we can conclude that it tries to call and invalid function pointer. To replace the invalid pointer we introduce a new function, `setMcontextEip`, which is very similar to `getMcontextEip` and it knows to set the Eip for the different supported OS's. After printing the trace we retrieve the old `Eip` value. (cherry picked from commit 0bf90d9)

…edis#11298) If Redis crashes due to calling an invalid function pointer, the `backtrace` function will try to dereference this invalid pointer which will cause a crash inside the crash report and will kill the processes without having all the crash report information. Example: ``` === REDIS BUG REPORT START: Cut & paste starting from here === 198672:M 19 Sep 2022 18:06:12.936 # Redis 255.255.255 crashed by signal: 11, si_code: 1 198672:M 19 Sep 2022 18:06:12.936 # Accessing address: 0x1 198672:M 19 Sep 2022 18:06:12.936 # Crashed running the instruction at: 0x1 // here the processes is crashing ``` This PR tries to fix this crash be: 1. Identify the issue when it happened. 2. Replace the invalid pointer with a pointer to some dummy function so that `backtrace` will not crash. I identification is done by comparing `eip` to `info->si_addr`, if they are the same we know that the crash happened on the same address it tries to accesses and we can conclude that it tries to call and invalid function pointer. To replace the invalid pointer we introduce a new function, `setMcontextEip`, which is very similar to `getMcontextEip` and it knows to set the Eip for the different supported OS's. After printing the trace we retrieve the old `Eip` value.

alecxvs · 2023-12-07T20:54:15Z

Was the fix backported, and is there any progress on correcting the CVE status? I'm running 6.2.14 but it's being flagged for this despite being disputed, and if it was already backported in the version I'm running, it may also be a false positive and the NIST db hasn't been updated with the backport version yet.

oranagra · 2023-12-09T10:33:55Z

yes, it was backported and part of 6.2.8 and later.

MeirShpilraien marked this pull request as draft September 21, 2022 13:39

Fix compilation warning

7964103

Better code usibility

190b826

MeirShpilraien marked this pull request as ready for review September 21, 2022 16:04

MeirShpilraien requested a review from oranagra September 21, 2022 16:04

oranagra reviewed Sep 22, 2022

View reviewed changes

src/debug.c Outdated Show resolved Hide resolved

src/debug.c Outdated Show resolved Hide resolved

src/debug.c Outdated Show resolved Hide resolved

MeirShpilraien and others added 2 commits September 22, 2022 13:33

Apply suggestions from code review

d3e178f

Co-authored-by: Oran Agra <oran@redislabs.com>

Review fixes

e5f3673

MeirShpilraien requested a review from oranagra September 22, 2022 10:35

Fix compilation failure.

df795d0

oranagra approved these changes Sep 22, 2022

View reviewed changes

yossigo reviewed Sep 24, 2022

View reviewed changes

oranagra changed the title ~~Avoid crash on crash report.~~ Avoid crash on crash report when an invalid function pointer was used Sep 29, 2022

oranagra changed the title ~~Avoid crash on crash report when an invalid function pointer was used~~ Avoid crash on crash report when a bad function pointer was called Sep 29, 2022

oranagra merged commit 0bf90d9 into redis:unstable Sep 29, 2022

esteinerMW added a commit to esteinerMW/redis that referenced this pull request Nov 3, 2022

Back port redis#11298 fix from unstable branch to Redis 6.2

b66e50c

esteinerMW mentioned this pull request Nov 3, 2022

Back port #11298 fix from unstable branch to Redis 6.2 #11473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid crash on crash report when a bad function pointer was called #11298

Avoid crash on crash report when a bad function pointer was called #11298

MeirShpilraien commented Sep 21, 2022 •

edited

Loading

oranagra commented Sep 21, 2022 •

edited

Loading

oranagra commented Sep 21, 2022

devnexen commented Sep 21, 2022

yossigo left a comment

oranagra commented Sep 24, 2022

yossigo commented Sep 28, 2022

oranagra commented Sep 29, 2022

iav20 commented Nov 14, 2022

oranagra commented Nov 14, 2022

esteinerMW commented Nov 14, 2022

yossigo commented Nov 15, 2022

alecxvs commented Dec 7, 2023

oranagra commented Dec 9, 2023

Avoid crash on crash report when a bad function pointer was called #11298

Avoid crash on crash report when a bad function pointer was called #11298

Conversation

MeirShpilraien commented Sep 21, 2022 • edited Loading

oranagra commented Sep 21, 2022 • edited Loading

oranagra commented Sep 21, 2022

devnexen commented Sep 21, 2022

yossigo left a comment

Choose a reason for hiding this comment

oranagra commented Sep 24, 2022

yossigo commented Sep 28, 2022

oranagra commented Sep 29, 2022

iav20 commented Nov 14, 2022

oranagra commented Nov 14, 2022

esteinerMW commented Nov 14, 2022

yossigo commented Nov 15, 2022

alecxvs commented Dec 7, 2023

oranagra commented Dec 9, 2023

MeirShpilraien commented Sep 21, 2022 •

edited

Loading

oranagra commented Sep 21, 2022 •

edited

Loading