1749279 - UndefinedBehaviorSanitizer: dom/crypto/KeyAlgorithmProxy.cpp:53:11: runtime error: load of value 58339, which is not a valid value for type 'mozilla::dom::KeyAlgorithmProxy::KeyAlgorithmType'

Status: RESOLVED FIXED

(Core :: DOM: Web Crypto, defect)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20211213-c9d499871dc8.

Backtrace:

dom/crypto/KeyAlgorithmProxy.cpp:53:11: runtime error: load of value 58339, which is not a valid value for type 'mozilla::dom::KeyAlgorithmProxy::KeyAlgorithmType'
    #0 0x7fbda4db8e7f in mozilla::dom::KeyAlgorithmProxy::ReadStructuredClone(JSStructuredCloneReader*) dom/crypto/KeyAlgorithmProxy.cpp:53:11
    #1 0x7fbda4db7f76 in mozilla::dom::CryptoKey::ReadStructuredClone(JSContext*, nsIGlobalObject*, JSStructuredCloneReader*) dom/crypto/CryptoKey.cpp:1111:31
    #2 0x7fbda39364b3 in mozilla::dom::CryptoKey_Binding::Deserialize(JSContext*, nsIGlobalObject*, JSStructuredCloneReader*) obj-build/dom/bindings/SubtleCryptoBinding.cpp:4078:43
    #3 0x7fbda2993f6b in mozilla::dom::StructuredCloneHolder::ReadFullySerializableObjects(JSContext*, JSStructuredCloneReader*, unsigned int) dom/base/StructuredCloneHolder.cpp:430:12
    #4 0x7fbda2996841 in mozilla::dom::StructuredCloneHolder::CustomReadHandler(JSContext*, JSStructuredCloneReader*, JS::CloneDataPolicy const&, unsigned int, unsigned int) dom/base/StructuredCloneHolder.cpp:1008:10
    #5 0x7fbdac01f982 in JSStructuredCloneReader::startRead(JS::MutableHandle<JS::Value>, js::gc::InitialHeap) js/src/vm/StructuredClone.cpp:2820:11
    #6 0x7fbdac008ae1 in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>, unsigned long) js/src/vm/StructuredClone.cpp:3232:8
    #7 0x7fbdac0081fa in ReadStructuredClone(JSContext*, JSStructuredCloneData const&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:703:12
    #8 0x7fbdac02743e in JS_ReadStructuredClone(JSContext*, JSStructuredCloneData const&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:3388:10
    #9 0x7fbda2993d18 in mozilla::dom::StructuredCloneHolder::ReadFromBuffer(nsIGlobalObject*, JSContext*, JSStructuredCloneData&, unsigned int, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:409:8
    #10 0x7fbda2993c34 in mozilla::dom::StructuredCloneHolder::ReadFromBuffer(nsIGlobalObject*, JSContext*, JSStructuredCloneData&, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:395:3
    #11 0x7fbda6921a37 in mozilla::dom::ipc::StructuredCloneData::Read(JSContext*, JS::MutableHandle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) /dom/ipc/StructuredCloneData.cpp:116:3
    #12 0x7fbda69196fc in mozilla::dom::ipc::StructuredCloneData::Read(JSContext*, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /dom/ipc/StructuredCloneData.cpp:104:3    
    #13 0x7fbd9d8188af in FuzzingRunDomSC(unsigned char const*, unsigned long) dom/base/fuzztest/FuzzStructuredClone.cpp:62:10

To reproduce the issue, perform the following steps:

1. Download the attached testcase, save as "test.bin".
   2a. Build with --enable-fuzzing (requires Clang and ASan, also build gtests using ./mach gtest dontruntests).
   2b. Alternatively you can download builds from TC using python -mfuzzfetch -a --fuzzing --target firefox gtest (see https://github.com/MozillaSecurity/fuzzfetch).
2. Run FUZZER=StructuredCloneReaderDOM objdir/dist/bin/firefox test.bin

I quickly looked into this and I think this isn't harmful. In this case, we are casting an unchecked uint32_t directly to the enum type which is undefined behavior but I can't see anything immediately going wrong because of it. Nevertheless, this needs to be fixed (the compiler can for example make the last case statement in this switch the default and assume the type without checking, leading to weird results).

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Missing test for comment 0.

Comment 3

[Christian Holler (:decoder)]

Attachment: Testcase for comment 0

Comment 4

[Christian Holler (:decoder)]

Attachment: Bug 1749279 - Validate KeyAlgorithmType before cast. r?keeler

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98e6170edea6
Validate KeyAlgorithmType before cast. r=keeler

Comment 6

[Sandor Molnar[:smolnar]]

Backed out changeset 98e6170edea6 (bug 1749279) for causing bp-hybrid build bustages in dom/crypto/KeyAlgorithmProxy.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/3e26fe5f65f41934282adb30db0c4d7f0ac8d351

Push with failures

Failure log

 ERROR -  /builds/worker/checkouts/gecko/dom/crypto/KeyAlgorithmProxy.cpp:106:10: error: 'return' will never be executed [-Werror,-Wunreachable-code-return]
[task 2022-01-31T21:02:00.691Z] 21:02:00     INFO -    return false;
[task 2022-01-31T21:02:00.691Z] 21:02:00     INFO -           ^~~~~
[task 2022-01-31T21:02:00.691Z] 21:02:00     INFO -  1 error generated.
[task 2022-01-31T21:02:00.691Z] 21:02:00    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:658: KeyAlgorithmProxy.o] Error 1
[task 2022-01-31T21:02:00.691Z] 21:02:00     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/crypto'
[task 2022-01-31T21:02:00.692Z] 21:02:00    ERROR -  gmake[3]: *** [/builds/worker/checkouts/gecko/config/recurse.mk:72: dom/crypto/target-objects] Error 2
[task 2022-01-31T21:02:00.692Z] 21:02:00     INFO -  gmake[3]: *** Waiting for unfinished jobs....

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73db3dae2f11
Validate KeyAlgorithmType before cast. r=keeler

Comment 8

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/73db3dae2f11