Opened 5 years ago

Last modified 23 months ago

#12418 assigned defect

TBBs with UBSan create lots of errors when running

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-security, TorBrowserTeam201711
Cc: arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When running TBBs (based on ESR 24) built with UBSan we get loads of errors which look like:

/home/ubuntu/build/tor-browser/js/src/jsobj.cpp:1008:17: runtime error: load of value 120, which is not a valid value for type 'bool'
pkix_pl_object.c:580:31: runtime error: left shift of 4276994303 by 32 places cannot be represented in type 'long int'
/home/ubuntu/build/tor-browser/db/sqlite3/src/sqlite3.c:62742:22: runtime error: left shift of 173 by 24 places cannot be represented in type 'int'
/home/ubuntu/build/tor-browser/layout/style/nsCSSParser.cpp:4861:53: runtime error: load of value 128, which is not a valid value for type 'bool'
/home/ubuntu/build/tor-browser/layout/style/../base/nsStyleConsts.h:27:12: runtime error: load of value 4, which is not a valid value for type 'Side'
/home/ubuntu/build/tor-browser/layout/style/nsCSSParser.cpp:6181:3: runtime error: load of value 4, which is not a valid value for type 'Side'
/home/ubuntu/build/tor-browser/layout/style/nsCSSParser.cpp:7962:5: runtime error: load of value 4, which is not a valid value for type 'Side'
/home/ubuntu/build/tor-browser/dom/workers/Workers.h:81:18: runtime error: load of value 4294967295, which is not a valid value for type 'JSGCParamKey'
/home/ubuntu/build/tor-browser/dom/workers/Workers.h:135:23: runtime error: load of value 4294967295, which is not a valid value for type 'JSGCParamKey'

Child Tickets

Change History (23)

comment:1 Changed 5 years ago by gk

09:06 < GeKo> oh, btw. are you running (semi-)regular UBSan builds as well? I am 
              getting loads of "runtime error: load of value X, which is not a value 
              for type Y" errors.
09:06 < decoder> no, im not. the problem with ubsan is that most errors it reports 
                 are likely not going to be fixed by developers
09:07 < decoder> it's really hard to reason with devs about fixing those
09:07 < GeKo> ugh
09:07 < decoder> in some cases (e.g. signed overflows, float overflows, in layout/) 
                 developers said it's well known and wont be fixed
09:07 < decoder> the whole layout/ engine does signed overflows all the time and 
                 doesnt care about the result
09:08 < decoder> they say worst thing that can happen is wrong layouting
09:08 < GeKo> :(
09:09 < decoder> GeKo: ive found signed overflows before though with ubsan, that had 
                 a certain security impact
09:09 < decoder> problem is, with all those messages from layout/ and some other 
                 components that we need to ignore, we need a working blacklist 
                 mechanism
09:10 < decoder> so we can ignore those problems

comment:2 Changed 5 years ago by erinn

Keywords: needs-triage added

comment:3 Changed 5 years ago by gk

Keywords: needs-triage removed

comment:4 Changed 4 years ago by gk

Keywords: tbb-hardening added

comment:5 Changed 4 years ago by gk

Parent ID: #10599
Severity: Normal

comment:6 Changed 4 years ago by gk

Keywords: tbb-hardened added; tbb-hardening removed

comment:7 Changed 3 years ago by bugzilla

Component: Applications/Tor bundles/installationApplications/Tor Browser
Owner: changed from erinn to tbb-team
Status: newassigned

Maybe, it's better to start using UBSan on FF's components step by step (JS, NSS, etc).

comment:8 in reply to:  7 Changed 3 years ago by cypherpunks

Replying to bugzilla:

Maybe, it's better to start using UBSan on FF's components step by step (JS, NSS, etc).

I'd start with the image decoders. I know of at least one 0day being traded actively which is exploitable in Tor Browser in the highest security setting, and none of the people who I trade with are going to report it (neither can I). But UBSan is very likely to mitigate it, if trapped to ud2 with -fsanitize-undefined-trap-on-error, as well as others. After that, NSS is probably the most important, because it can't be turned off. JS has a huge surface area, but it can be disabled by the slider.

If I have free time, I'll try building FF with the image decoders using UBSan, but I'd really rather it if someone else who's already testing this stuff out do it since I've been very busy with other things (I might take a while).

comment:9 Changed 3 years ago by arthuredelstein

Cc: arthuredelstein added

comment:10 Changed 2 years ago by cypherpunks

If Tor Browser is ever compiled with Clang (if that's even possible), the UBSan subset whitelist supports far more fine granularity, permitting the whitelisting of individual UBSan subsets within not only single files, but within single functions as well. This way, if a large file has several functions that violate, say, object-size, then those functions can avoid being instrumented, without having to disable it on the entire source file. More information about Clang's "special case list" is on https://clang.llvm.org/docs/SanitizerSpecialCaseList.html.

Has anyone started working on at least instrumenting individual FF components, as suggested above?

comment:11 in reply to:  10 ; Changed 2 years ago by tom

Replying to cypherpunks:

Has anyone started working on at least instrumenting individual FF components, as suggested above?

Kind of. Mozilla spend a considerable amount of person-time playing with UBSAN.

The conclusion was that some tests are valuable and should be used (bounds, pointer-overflow, vptr although this requires RTTI).

But that others (signed and unsigned overflow) caused a gratuitous amount of false positives (largely in the graphics and layout areas but in general all over the place) and it's infeasible to whitelist them all. We had someone spend a month on this and using his whitelist we brought the number of reports down from the hundred of thousands down to the mere thousands - but even then it was with a ton of effort and had a ton of effort to go.

So I think the path forward is to turn on UBSAN on the whole browser, run it through something like the web platform tests or Mozilla's usual unit tests, and slowly increase the number of UBSAN tests one by one. When we hit one that causes too many false positives, we turn it back off and investigate turning it on for an individual component (like image decoders.)

Also I would suggest the path forward for this is in Mozilla's court, rather than Tor's. Not that Tor has to wait for Mozilla, only that making use of Mozilla's infrastructure will make it considerably easier. Tor devs have access to that, and if any cypherpunks want access, I think the only thing needed is a few contributions* that I can point to and say "This person is doing good work, let's give them access to run their tests on our task runner".

\* Contributions such as looking at and improving UBSAN? :)

Here's where our stuff is:

comment:12 in reply to:  11 ; Changed 2 years ago by cypherpunks

Replying to tom:

Replying to cypherpunks:

Has anyone started working on at least instrumenting individual FF components, as suggested above?

Kind of. Mozilla spend a considerable amount of person-time playing with UBSAN.

Wow! I had no idea so much work has already been put into this task! This will be very helpful.

The conclusion was that some tests are valuable and should be used (bounds, pointer-overflow, vptr although this requires RTTI).

But that others (signed and unsigned overflow) caused a gratuitous amount of false positives (largely in the graphics and layout areas but in general all over the place) and it's infeasible to whitelist them all. We had someone spend a month on this and using his whitelist we brought the number of reports down from the hundred of thousands down to the mere thousands - but even then it was with a ton of effort and had a ton of effort to go.

Unfortunately, those are some of the most important types of UB that must be prevented. An alternative (mutually exclusive due to incompatibilities with internal symbol names, or something of that sort), if suitable manpower is present, is to instrument important parts of FF with the PaX Size Overflow plugin (see https://forums.grsecurity.net/viewtopic.php?f=7&t=3043). It provides better protection than UBSAN for this specific issue.

So I think the path forward is to turn on UBSAN on the whole browser, run it through something like the web platform tests or Mozilla's usual unit tests, and slowly increase the number of UBSAN tests one by one. When we hit one that causes too many false positives, we turn it back off and investigate turning it on for an individual component (like image decoders.)

I had assumed that the amount of UB would be so great that it would be infeasible to do this in any reasonable amount of time. I still feel like instrumenting individual components of the browser would be easier.

Also I would suggest the path forward for this is in Mozilla's court, rather than Tor's. Not that Tor has to wait for Mozilla, only that making use of Mozilla's infrastructure will make it considerably easier. Tor devs have access to that, and if any cypherpunks want access, I think the only thing needed is a few contributions* that I can point to and say "This person is doing good work, let's give them access to run their tests on our task runner".

I tend to avoid Mozilla's ticket system due to their excessively bureaucratic nature, and their tendency to put security as a low priority. All my Firefox-related contributions have been made here (though admittedly I have made more contributions for Tor itself, and relatively few for Firefox).

comment:13 in reply to:  12 ; Changed 2 years ago by tom

Replying to cypherpunks:

Replying to tom:

The conclusion was that some tests are valuable and should be used (bounds, pointer-overflow, vptr although this requires RTTI).

But that others (signed and unsigned overflow) caused a gratuitous amount of false positives (largely in the graphics and layout areas but in general all over the place) and it's infeasible to whitelist them all. We had someone spend a month on this and using his whitelist we brought the number of reports down from the hundred of thousands down to the mere thousands - but even then it was with a ton of effort and had a ton of effort to go.

Unfortunately, those are some of the most important types of UB that must be prevented. An alternative (mutually exclusive due to incompatibilities with internal symbol names, or something of that sort), if suitable manpower is present, is to instrument important parts of FF with the PaX Size Overflow plugin (see https://forums.grsecurity.net/viewtopic.php?f=7&t=3043). It provides better protection than UBSAN for this specific issue.

Hmmmmm. I don't know I will have to investigate.

So I think the path forward is to turn on UBSAN on the whole browser, run it through something like the web platform tests or Mozilla's usual unit tests, and slowly increase the number of UBSAN tests one by one. When we hit one that causes too many false positives, we turn it back off and investigate turning it on for an individual component (like image decoders.)

I had assumed that the amount of UB would be so great that it would be infeasible to do this in any reasonable amount of time. I still feel like instrumenting individual components of the browser would be easier.

What do you mean by "amount of UBSAN"? There are some checks that should have basically no false-positives (like pointer overflow) - those should be feasible for whole-browser I think.

Instrumenting components with more verbose tests (like int overflows) is definitely valuable though!

Mind you, Mozilla's not going to ship Firefox with UBSAN enabled, we'll just run tests with it to catch issues. Maybe Tor would ship something with UBSAN (??) but maybe not since I don't think you can enable both ASAN and UBSAN.

Also I would suggest the path forward for this is in Mozilla's court, rather than Tor's. Not that Tor has to wait for Mozilla, only that making use of Mozilla's infrastructure will make it considerably easier. Tor devs have access to that, and if any cypherpunks want access, I think the only thing needed is a few contributions* that I can point to and say "This person is doing good work, let's give them access to run their tests on our task runner".

I tend to avoid Mozilla's ticket system due to their excessively bureaucratic nature, and their tendency to put security as a low priority. All my Firefox-related contributions have been made here (though admittedly I have made more contributions for Tor itself, and relatively few for Firefox).

Well, It's a big org, we're not all bad ;) But I hear you loud and clear. My main point was not "Try and get Mozilla to take your patches" but rather "You can almost certainly make use of Mozilla's infrastructure to do experimental runs and examine the output." For example, you could queue up 10 jobs that turn on UBSAN for 10 individual components, and run them all at once.

comment:14 Changed 2 years ago by arthuredelstein

I started look into the ubsan errors by adding -fsanitize-undefined to a mozconfig in mozilla-central:
https://github.com/arthuredelstein/tor-browser/commit/ubsan3

I pushed to the try server to run all unit tests and talos tests on linux, linux64 (debug and optimized)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43f94f28e54232cd9fec8abb81b871121939aefd

Then I wrote scripts to download all logs files from this try server run, extract all "runtime errors" reported by ubsan in the logs, and then group the runtime errors by their location in the codebase.

https://github.com/arthuredelstein/firefox-ubsan-errors

In total there were some 170,000 runtime errors reported in the logs, produced by 367 specific locations in the codebase. (Some locations caused thousands of 'runtime error' messages each.) I generated a summary table that shows these locations and a representative error message. Here it is in a Google doc spreadsheet:
https://docs.google.com/spreadsheets/d/1ISxhkwWVwa7HBVEd6gPTcynfMwaq-cmI_wQsiDZxLhc/edit?usp=sharing

And here is the same table in a tab-delimited text file:
https://gist.github.com/arthuredelstein/a208b1d7334c9e1d669308b9cd06f96b

My next steps are to generate the same table for a clang -fsanitize=undefined build, and then start patching and/or whitelisting all functions in given category of ubsan error (such as integer overflow). If Mozilla can accept these patches, then I imagine we can turn on ubsan subflags in the mozilla-central debug builds and also turn them on by default in Tor Browser (alpha to begin with).

Last edited 2 years ago by arthuredelstein (previous) (diff)

comment:15 Changed 2 years ago by gk

Keywords: TorBrowserTeam201708 added; tbb-hardened removed

comment:16 Changed 2 years ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:17 Changed 2 years ago by arthuredelstein

Here is a summary of my recent efforts on this ticket.

First, as a proof of principle, I patched most of the enums in mozilla-central that were showing ubsan errors. My branch is here:

https://github.com/arthuredelstein/tor-browser/commits/ubsan-enum1

There are a few further enum error locations I need to fix, and then I think I can propose adding -fsanitize=enum to Firefox debug builds and Tor Browser alpha builds.

However, enum errors are not a likely source of security problems. Rather a much more dangerous undefined behavior caught by ubsan are the signed integer overflows.

So I pushed a new try server run for linux on mozilla-central, with -fsanitize=signed-integer-overflow enabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=876a50a1c1122c02186f329f04d020aafa83c5f2

There were a total of 15685 signed integer overflow events in 221 locations:
https://docs.google.com/spreadsheets/d/1yluXK-9V4_0C40WEZn3fS5LCOG_az_I6HcuptLBZ-qM/edit#gid=0 (Here's the raw file: https://gist.github.com/arthuredelstein/f711d25c997028a59414f42dc97ec7e3)

I observed that 140 of the overflow sites (of 221) are located in layout/*. (These comprise nearly all of the undefined behaviors of any kind I found in layout/* in comment:14. A further 15 are found in gfx/cairo/*, which is an external library that I suppose will need to be patched upstream.

How do we deal with these errors? My currently thinking is, first we should suppress every one of these overflow warnings by patching or ignoring them. Then we can turn on

-fsanitize=signed-integer-overflow -fno-sanitize-recover=signed-integer-overflow

in the Mozilla debug build to prevent new overflow errors from being introduced. Later, as time allows, we can go back and try to stop ignoring errors in other sections of the code and fix them instead.

So I decided to try to simplify the problem by temporarily suppressing errors in layout/* and gfx/cairo/*, by adding -fno-sanitize=signed-integer-overflow flags in their respective moz.build files. (I also patched a couple of locations in code to remove their overflows.) Here is the branch and the results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=292ac14828e642d62e7fbc7fab1f719bc98c07bc

There are about 45 remaining overflow locations in the code. Many of these seem to be signed integer additions where one of the addends has the value (231 - delta), where delta is a small integer between 1 and 1000. The other addend is typically a small integer, but greater than delta, thus resulting in an overflow. So my next tasks are to work out (1) where some of these integers close to INT_MAX are coming from, and (2) how to handle that type of error.

My hope is we can use Mozilla’s CheckedInt mechanism to handle these errors safely (instead of aborting via ubsan). Depending on the performance impact, we might need to write a mozilla::CheckedInt-based macro that only runs if we are using signed integer overflows.

Other thoughts: in the future, we could investigate whether fuzzing the build will find more integer overflows that aren’t incidentally exposed by Mozilla’s regression tests. I believe some fuzzing of nightly builds is already being done, so turning on ubsan flags may already have interesting results.

Last edited 2 years ago by arthuredelstein (previous) (diff)

comment:18 in reply to:  13 Changed 2 years ago by cypherpunks

Replying to tom:

What do you mean by "amount of UBSAN"? There are some checks that should have basically no false-positives (like pointer overflow) - those should be feasible for whole-browser I think.

Amount of UB (undefined behavior), not amount of UBSAN. And yes, some checks seem to very compatible with many programs (-fsanitize=object-size for example). Those should be the first tested, though instrumenting the image decoders as thoroughly as possible should be high priority.

Just a side-note, but -ftrapv and friends interfere with -fsanitize=signed-integer-overflow, such that the former disables the latter, even though it is easier to bypass. They should never be combined.

Instrumenting components with more verbose tests (like int overflows) is definitely valuable though!

Mind you, Mozilla's not going to ship Firefox with UBSAN enabled, we'll just run tests with it to catch issues. Maybe Tor would ship something with UBSAN (??) but maybe not since I don't think you can enable both ASAN and UBSAN.

You can enable both ASAN and UBSAN, but ASAN is not intended for use in production systems for security as the complex runtime can introduce vulnerabilities, and it's quite easy to bypass anyway considering it's looking for unintentional bugs that aren't trying to be stealthy. UBSAN on the other hand is fine for production use.

Well, It's a big org, we're not all bad ;) But I hear you loud and clear. My main point was not "Try and get Mozilla to take your patches" but rather "You can almost certainly make use of Mozilla's infrastructure to do experimental runs and examine the output." For example, you could queue up 10 jobs that turn on UBSAN for 10 individual components, and run them all at once.

I have a small cluster of computers which I can use to do the test myself, so long as I have the code for the unit tests. Getting trusted by Mozilla to the point where I can use their test infrastructure is not something I'm eager to commit to.

On a side-note, I've already tested Tor (not the browser) with -fsanitize=object-size, and it works perfectly.

Last edited 2 years ago by cypherpunks (previous) (diff)

comment:19 Changed 2 years ago by tom

Cool! And good work arthur. I signed up to tackle https://bugzilla.mozilla.org/show_bug.cgi?id=1402316 which should lay a framework for silencing individual errors we don't care about.

comment:20 in reply to:  19 Changed 2 years ago by arthuredelstein

Replying to tom:

Cool! And good work arthur. I signed up to tackle https://bugzilla.mozilla.org/show_bug.cgi?id=1402316 which should lay a framework for silencing individual errors we don't care about.

Thanks, Tom. I commented on that ticket.

comment:21 Changed 2 years ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:22 Changed 2 years ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

comment:23 Changed 23 months ago by arthuredelstein

As a status report: I have started work on trying to fix ubsan runtime errors in Firefox and Tor Browser. Here's my script to download all log files from the Mozilla task cluster, and extract, deduplicate, and categorize the ubsan errors from a try server run:
https://github.com/arthuredelstein/firefox-ubsan-errors

I decided to first focus on he category of ubsan enum errors as a proof of principle. Here's the parent ticket:
https://bugzilla.mozilla.org/show_bug.cgi?id=1404547

I opened and fixed the following tickets:

I also opened the following tickets:

There remain four more ubsan enum error locations. I expect two of these can be fixed, and the other two will need to be whitelisted using -fno-sanitize=enum. Then we can add -fsanitize=enum for the overall Firefox/TBB debug build.

Then the next steps will be to fix other types of ubsan runtime errors and then turn on aborts for them in the debug builds so that future ubsan errors can be discovered during automated test runs.

Last edited 23 months ago by arthuredelstein (previous) (diff)
Note: See TracTickets for help on using tickets.