1201666 - Missing status check can cause variant_storage_traits<uint8_t[], false>::storage_conversion to silently elide data

Status: NEW

(Core :: SQLite and Embedded Database Bindings, defect, P3)

Description

[q1]

variant_storage_traits<uint8_t[], false>::storage_conversion (storage\src\Variant.h) does not check the status from its call to FallibleTArray::AppendElements, nor does it return the status to its callers. It thus can silently elide one or more elements from its output on OOM. This can compromise security. For example, EncodeKeysFunction::OnFunctionCall (dom\indexedDB\ActorsParent.cpp) uses storage::BlobVariant to extract an encryption key. But BlobVariant isa Variant<uint8_t[], false> (variant.h), and Variant<uint8_t[], false>::Variant uses variant_storage_traits<uint8_t[], false> to extract the key.

I have not analyzed others of storage_conversion's callers for their security impacts.


Details:
--------

The bugs are on variant.h lines 266-67 (no status check) and 262 (void return value):

257: template < >
258: struct variant_storage_traits<uint8_t[], false>
259: {
260:   typedef std::pair<const void *, int> ConstructorType;
261:   typedef FallibleTArray<uint8_t> StorageType;
262:   static inline void storage_conversion(ConstructorType aBlob, StorageType* _outData)
263:   {
264:     _outData->Clear();
265:     _outData->SetCapacity(aBlob.second);
266:     (void)_outData->AppendElements(static_cast<const uint8_t *>(aBlob.first),
267:                                    aBlob.second);
268:   }
...

The indirect caller EncodeKeysFunction::OnFunctionCall (dom\indexedDB\ActorsParent.cpp) does:

2202:  NS_IMETHOD
2203:  OnFunctionCall(mozIStorageValueArray* aArguments,
2204:                 nsIVariant** aResult) override
2205:  {
...
2244:    const nsCString& buffer = key.GetBuffer();
2245:
2246:    std::pair<const void *, int> data(static_cast<const void*>(buffer.get()),
2247:                                      int(buffer.Length()));
2248:
2249:    nsCOMPtr<nsIVariant> result = new mozilla::storage::BlobVariant(data);
2250:
2251:    result.forget(aResult);
2252:    return NS_OK;
2253:  }

Line 2249 calls BlobVariant, which:

441: typedef Variant<uint8_t[], false> BlobVariant;

defines as:

378: template <typename DataType, bool Adopting=false>
379: class Variant final : public Variant_base
380: {
...
387:   explicit Variant(const typename variant_storage_traits<DataType, Adopting>::ConstructorType aData)
388:   {
389:     variant_storage_traits<DataType, Adopting>::storage_conversion(aData, &mData);
390:   }
...

which leads back to variant.h line 257, above.

9/3/2015's Aurora has the same defect, though the redundant call to SetCapacity on variant.h line 265 has been removed: http://hg.mozilla.org/releases/mozilla-aurora/file/1d9d6a7a958a/storage/Variant.h .

Comment 1

[Andrew McCreight [:mccr8]]

Kyle, do you think this could cause a security issue?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I doubt this is a security issue, because the fallible AppendElements either appends the whole thing or nothing.

Comment 3

[q1]

(In reply to Kyle Huey [:khuey] ([e-mail]) (UTC+8 and delayed responses until 9/21) from comment #2)
> I doubt this is a security issue, because the fallible AppendElements either
> appends the whole thing or nothing.

If it appends nothing, some caller unexpectedly gets a null key. If it uses that key without checking it, whatever operation it performs will be either trivially weak (encryption, signing, hashing) or incorrect (decryption, signature verification).

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The "key" here is not a cryptographic key, it's a key in the key-value store sense.

Comment 5

[q1]

(In reply to Kyle Huey [:khuey] ([e-mail]) (UTC+8 and delayed responses until 9/21) from comment #4)
> The "key" here is not a cryptographic key, it's a key in the key-value store
> sense.

Ack! I should have looked deeper. Presumably then the security impact depends upon what the retrieval key is used for, and how lookup failures are handled.

Comment 6

[Marco Bonardo [:mak]]

I agree with comment 2.

I think we want to preserve a fallible behavior here, because blobs can be large and we likely don't want a failed allocation to be fatal here.
Since I doubt we can do this at the constructor level without making this API fancy, I guess the best we can do is track a status mIsValid property that will likely be true for everything but the non-adopted blob variant (where it will depend on the result of AppendElements).
Then we can use it to bailout from asArray with an NS_ERROR_OUT_OF_MEMORY.
I'm not sure how we could write an automated test for that though.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't think you can realistically do automated testing of OOM scenarios.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Opening this up since it isn't a security issue.