⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 276510 in webkit


Ignore:
Timestamp:
Apr 23, 2021, 12:06:25 PM (5 years ago)
Author:
Darin Adler
Message:

Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes
​https://bugs.webkit.org/show_bug.cgi?id=224984

Reviewed by Sam Weinig.

Source/WebCore:

  • platform/network/cf/CertificateInfoCFNet.cpp:

(WTF::Persistence::decodeCFData): Removed unneeded check for zero size. Removed code that
locally allocates a vector before bufferIsLargeEnoughToContain is called. Instead use
bufferPointerForDirectRead, which makes does the buffer size check, and pass the pointer
directly to CFDataCreate.

Source/WebKit:

  • Platform/IPC/ArgumentCoders.h: Remove the calls to

HashMap::reserveInitialCapacity and HashSet::reserveInitialCapacity, based
on number read in from the decoder. This means there will be more wasted
memory in these HashMap and HashSet objects, so we have to test to make
sure this does not create a performance problem. But without this check,
we are trying to allocate memory based on an unstrusted size.

  • Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:

(IPC::ArgumentCoder<RefPtr<ApplePayError>>::encode): Removed the coder
for a Vector of these RefPtr, replaced it with a coder for an individual one,
allowing the Vector ArgumentCoder template to handle vector size and construction.
One benefit is that this adds in a shrinkToFit and prevents us from making any
separate mistake about pre-sizing the Vector here since we use shared code.
(IPC::ArgumentCoder<RefPtr<ApplePayError>>::decode): Ditto.

  • Shared/WebCoreArgumentCoders.cpp:

(IPC::ArgumentCoder<RefPtr<SecurityOrigin>>::encode): Ditto.
(IPC::ArgumentCoder<RefPtr<SecurityOrigin>>::decode): Ditto.
(IPC::ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::encode):
(IPC::ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode):
Removed unnecessary specialization for the KeyStatusVector. There is already
an ArgumentCoder for Vector, for std::pair, for Ref<SharedBuffer>, and for
enumerations like CDMKeyStatus, so there's no need to have a specialized
coder for this. This function that we are removing had a call to
reserveInitialCapacity, but the Vector ArgumentCoder template does not.

  • Shared/WebCoreArgumentCoders.h: Replaced the

ArgumentCoder<Vector<RefPtr<WebCore::ApplePayError>>> specialization with
ArgumentCoder<RefPtr<WebCore::ApplePayError>>. Removed the
ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector> specialization.

Source/WTF:

  • wtf/persistence/PersistentDecoder.cpp:

(WTF::Persistence::Decoder::bufferPointerForDirectRead): Added.
(WTF::Persistence::Decoder::decodeFixedLengthData): Refactor to use bufferPointerForDirectRead.

  • wtf/persistence/PersistentDecoder.h: Added bufferPointerForDirectRead function for use in the

rare cases where we want to read directly out of the decoder buffer, rather than writing to a
passed-in pointer. Also did a small refactoring of bufferIsLargeEnoughToContain to use &&
rather than an if statement.

Location:
trunk/Source
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WTF/ChangeLog

    r276506 r276510  
     12021-04-23  Darin Adler  <darin@apple.com>
     2
     3        Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes
     4        https://bugs.webkit.org/show_bug.cgi?id=224984
     5
     6        Reviewed by Sam Weinig.
     7
     8        * wtf/persistence/PersistentDecoder.cpp:
     9        (WTF::Persistence::Decoder::bufferPointerForDirectRead): Added.
     10        (WTF::Persistence::Decoder::decodeFixedLengthData): Refactor to use bufferPointerForDirectRead.
     11
     12        * wtf/persistence/PersistentDecoder.h: Added bufferPointerForDirectRead function for use in the
     13        rare cases where we want to read directly out of the decoder buffer, rather than writing to a
     14        passed-in pointer. Also did a small refactoring of bufferIsLargeEnoughToContain to use &&
     15        rather than an if statement.
     16
    1172021-04-23  Chris Dumez  <cdumez@apple.com>
    218

  • trunk/Source/WTF/wtf/persistence/PersistentDecoder.cpp

    r259922 r276510  
    11/*
    2  * Copyright (C) 2010, 2011, 2014 Apple Inc. All rights reserved.
     2 * Copyright (C) 2010-2021 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4848}
    4949
    50 bool Decoder::decodeFixedLengthData(uint8_t* data, size_t size)
     50const uint8_t* Decoder::bufferPointerForDirectRead(size_t size)
    5151{
    5252    if (!bufferIsLargeEnoughToContain(size))
    53         return false;
     53        return nullptr;
    5454
    55     memcpy(data, m_bufferPosition, size);
     55    auto data = m_bufferPosition;
    5656    m_bufferPosition += size;
    5757
    5858    Encoder::updateChecksumForData(m_sha1, data, size);
     59    return data;
     60}
     61
     62bool Decoder::decodeFixedLengthData(uint8_t* data, size_t size)
     63{
     64    auto buffer = bufferPointerForDirectRead(size);
     65    if (!buffer)
     66        return false;
     67    memcpy(data, buffer, size);
    5968    return true;
    6069}

  • trunk/Source/WTF/wtf/persistence/PersistentDecoder.h

    r259980 r276510  
    11/*
    2  * Copyright (C) 2014 Apple Inc. All rights reserved.
     2 * Copyright (C) 2014-2021 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    8282    {
    8383        static_assert(std::is_arithmetic<T>::value, "Type T must have a fixed, known encoded size!");
     84        return numElements <= std::numeric_limits<size_t>::max() / sizeof(T) && bufferIsLargeEnoughToContain(numElements * sizeof(T));
     85    }
    8486
    85         if (numElements > std::numeric_limits<size_t>::max() / sizeof(T))
    86             return false;
    87 
    88         return bufferIsLargeEnoughToContain(numElements * sizeof(T));
    89     }
     87    WTF_EXPORT_PRIVATE WARN_UNUSED_RETURN const uint8_t* bufferPointerForDirectRead(size_t numBytes);
    9088
    9189    static constexpr bool isIPCDecoder = false;
    … …  
    104102}
    105103}
    106 

  • trunk/Source/WebCore/ChangeLog

    r276502 r276510  
     12021-04-23  Darin Adler  <darin@apple.com>
     2
     3        Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes
     4        https://bugs.webkit.org/show_bug.cgi?id=224984
     5
     6        Reviewed by Sam Weinig.
     7
     8        * platform/network/cf/CertificateInfoCFNet.cpp:
     9        (WTF::Persistence::decodeCFData): Removed unneeded check for zero size. Removed code that
     10        locally allocates a vector before bufferIsLargeEnoughToContain is called. Instead use
     11        bufferPointerForDirectRead, which makes does the buffer size check, and pass the pointer
     12        directly to CFDataCreate.
     13
    1142021-04-23  Chris Dumez  <cdumez@apple.com>
    215

  • trunk/Source/WebCore/platform/network/cf/CertificateInfoCFNet.cpp

    r275298 r276510  
    11/*
    2  * Copyright (C) 2010, 2015 Apple Inc. All rights reserved.
     2 * Copyright (C) 2010-2021 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    206206    Optional<uint64_t> size;
    207207    decoder >> size;
    208     if (!size)
    209         return WTF::nullopt;
    210208
    211209    if (UNLIKELY(!isInBounds<size_t>(*size)))
    212210        return WTF::nullopt;
    213    
    214     Vector<uint8_t> vector(static_cast<size_t>(*size));
    215     if (!decoder.decodeFixedLengthData(vector.data(), vector.size()))
    216         return WTF::nullopt;
    217 
    218     return adoptCF(CFDataCreate(nullptr, vector.data(), vector.size()));
     211
     212    auto pointer = decoder.bufferPointerForDirectRead(static_cast<size_t>(*size));
     213    if (!pointer)
     214        return WTF::nullopt;
     215
     216    return adoptCF(CFDataCreate(nullptr, pointer, *size));
    219217}
    220218

  • trunk/Source/WebKit/ChangeLog

    r276509 r276510  
     12021-04-23  Darin Adler  <darin@apple.com>
     2
     3        Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes
     4        https://bugs.webkit.org/show_bug.cgi?id=224984
     5
     6        Reviewed by Sam Weinig.
     7
     8        * Platform/IPC/ArgumentCoders.h: Remove the calls to
     9        HashMap::reserveInitialCapacity and HashSet::reserveInitialCapacity, based
     10        on number read in from the decoder. This means there will be more wasted
     11        memory in these HashMap and HashSet objects, so we have to test to make
     12        sure this does not create a performance problem. But without this check,
     13        we are trying to allocate memory based on an unstrusted size.
     14
     15        * Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:
     16        (IPC::ArgumentCoder<RefPtr<ApplePayError>>::encode): Removed the coder
     17        for a Vector of these RefPtr, replaced it with a coder for an individual one,
     18        allowing the Vector ArgumentCoder template to handle vector size and construction.
     19        One benefit is that this adds in a shrinkToFit and prevents us from making any
     20        separate mistake about pre-sizing the Vector here since we use shared code.
     21        (IPC::ArgumentCoder<RefPtr<ApplePayError>>::decode): Ditto.
     22        * Shared/WebCoreArgumentCoders.cpp:
     23        (IPC::ArgumentCoder<RefPtr<SecurityOrigin>>::encode): Ditto.
     24        (IPC::ArgumentCoder<RefPtr<SecurityOrigin>>::decode): Ditto.
     25        (IPC::ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::encode):
     26        (IPC::ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode):
     27        Removed unnecessary specialization for the KeyStatusVector. There is already
     28        an ArgumentCoder for Vector, for std::pair, for Ref<SharedBuffer>, and for
     29        enumerations like CDMKeyStatus, so there's no need to have a specialized
     30        coder for this. This function that we are removing had a call to
     31        reserveInitialCapacity, but the Vector ArgumentCoder template does not.
     32
     33        * Shared/WebCoreArgumentCoders.h: Replaced the
     34        ArgumentCoder<Vector<RefPtr<WebCore::ApplePayError>>> specialization with
     35        ArgumentCoder<RefPtr<WebCore::ApplePayError>>. Removed the
     36        ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector> specialization.
     37
    1382021-04-23  Kate Cheney  <katherine_cheney@apple.com>
    239

  • trunk/Source/WebKit/Platform/IPC/ArgumentCoders.h

    r275410 r276510  
    11/*
    2  * Copyright (C) 2010-2020 Apple Inc. All rights reserved.
     2 * Copyright (C) 2010-2021 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    515515
    516516        HashMapType hashMap;
    517         hashMap.reserveInitialCapacity(hashMapSize);
    518517        for (uint32_t i = 0; i < hashMapSize; ++i) {
    519518            Optional<KeyArg> key;
    … …  
    581580
    582581        HashSetType hashSet;
    583         hashSet.reserveInitialCapacity(hashSetSize);
    584582        for (uint64_t i = 0; i < hashSetSize; ++i) {
    585583            Optional<KeyArg> key;

  • trunk/Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm

    r276341 r276510  
    345345}
    346346
    347 void ArgumentCoder<Vector<RefPtr<ApplePayError>>>::encode(Encoder& encoder, const Vector<RefPtr<ApplePayError>>& errors)
    348 {
    349     encoder << static_cast<uint64_t>(errors.size());
    350     for (auto& error : errors) {
    351         encoder << !!error;
    352         if (error)
    353             encoder << *error;
    354     }
    355 }
    356 
    357 Optional<Vector<RefPtr<ApplePayError>>> ArgumentCoder<Vector<RefPtr<ApplePayError>>>::decode(Decoder& decoder)
    358 {
    359     uint64_t size;
    360     if (!decoder.decode(size))
    361         return WTF::nullopt;
    362 
    363     Vector<RefPtr<ApplePayError>> errors;
    364     for (uint64_t i = 0; i < size; ++i) {
    365         Optional<bool> isValid;
    366         decoder >> isValid;
    367         if (!isValid)
    368             return WTF::nullopt;
    369 
    370         RefPtr<ApplePayError> error;
    371         if (*isValid) {
    372             error = ApplePayError::decode(decoder);
    373             if (!error)
    374                 return WTF::nullopt;
    375         }
    376         errors.append(WTFMove(error));
    377     }
    378     return errors;
     347void ArgumentCoder<RefPtr<ApplePayError>>::encode(Encoder& encoder, const RefPtr<ApplePayError>& error)
     348{
     349    encoder << !!error;
     350    if (error)
     351        encoder << *error;
     352}
     353
     354Optional<RefPtr<ApplePayError>> ArgumentCoder<RefPtr<ApplePayError>>::decode(Decoder& decoder)
     355{
     356    Optional<bool> isValid;
     357    decoder >> isValid;
     358    if (!isValid)
     359        return WTF::nullopt;
     360
     361    RefPtr<ApplePayError> error;
     362    if (!*isValid)
     363        return { nullptr };
     364
     365    error = ApplePayError::decode(decoder);
     366    if (!error)
     367        return WTF::nullopt;
     368    return error;
    379369}
    380370

  • trunk/Source/WebKit/Shared/WebCoreArgumentCoders.cpp

    r276388 r276510  
    11/*
    2  * Copyright (C) 2011-2020 Apple Inc. All rights reserved.
     2 * Copyright (C) 2011-2021 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    29092909}
    29102910
    2911 void ArgumentCoder<Vector<RefPtr<SecurityOrigin>>>::encode(Encoder& encoder, const Vector<RefPtr<SecurityOrigin>>& origins)
    2912 {
    2913     encoder << static_cast<uint64_t>(origins.size());
    2914     for (auto& origin : origins)
    2915         encoder << *origin;
    2916 }
    2917    
    2918 bool ArgumentCoder<Vector<RefPtr<SecurityOrigin>>>::decode(Decoder& decoder, Vector<RefPtr<SecurityOrigin>>& origins)
    2919 {
    2920     uint64_t dataSize;
    2921     if (!decoder.decode(dataSize))
    2922         return false;
    2923 
    2924     for (uint64_t i = 0; i < dataSize; ++i) {
    2925         auto decodedOriginRefPtr = SecurityOrigin::decode(decoder);
    2926         if (!decodedOriginRefPtr)
    2927             return false;
    2928         origins.append(decodedOriginRefPtr.releaseNonNull());
    2929     }
    2930     origins.shrinkToFit();
    2931 
    2932     return true;
     2911void ArgumentCoder<RefPtr<SecurityOrigin>>::encode(Encoder& encoder, const RefPtr<SecurityOrigin>& origin)
     2912{
     2913    encoder << *origin;
     2914}
     2915   
     2916Optional<RefPtr<SecurityOrigin>> ArgumentCoder<RefPtr<SecurityOrigin>>::decode(Decoder& decoder)
     2917{
     2918    auto origin = SecurityOrigin::decode(decoder);
     2919    if (!origin)
     2920        return WTF::nullopt;
     2921    return origin;
    29332922}
    29342923
    … …  
    31503139    return makeOptional<WebCore::CDMInstanceSession::Message>({ type, buffer.releaseNonNull() });
    31513140}
    3152 
    3153 void ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::encode(Encoder& encoder, const WebCore::CDMInstanceSession::KeyStatusVector& keyStatuses)
    3154 {
    3155     encoder << static_cast<uint64_t>(keyStatuses.size());
    3156     for (auto& keyStatus : keyStatuses) {
    3157         RefPtr<SharedBuffer> key = keyStatus.first.copyRef();
    3158         encoder << key << keyStatus.second;
    3159     }
    3160 }
    3161 
    3162 Optional<WebCore::CDMInstanceSession::KeyStatusVector> ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode(Decoder& decoder)
    3163 {
    3164     uint64_t dataSize;
    3165     if (!decoder.decode(dataSize))
    3166         return WTF::nullopt;
    3167 
    3168     WebCore::CDMInstanceSession::KeyStatusVector keyStatuses;
    3169     keyStatuses.reserveInitialCapacity(dataSize);
    3170 
    3171     for (uint64_t i = 0; i < dataSize; ++i) {
    3172         RefPtr<SharedBuffer> key;
    3173         if (!decoder.decode(key) || !key)
    3174             return WTF::nullopt;
    3175 
    3176         WebCore::CDMInstanceSessionClient::KeyStatus status;
    3177         if (!decoder.decode(status))
    3178             return WTF::nullopt;
    3179 
    3180         keyStatuses.uncheckedAppend({ key.releaseNonNull(), status });
    3181     }
    3182     return keyStatuses;
    3183 }
    31843141#endif // ENABLE(ENCRYPTED_MEDIA)
    31853142
    … …  
    32473204
    32483205    if (!isEngaged)
    3249         return RefPtr<WebCore::ImageData>();
     3206        return { nullptr };
    32503207
    32513208    auto result = ArgumentCoder<Ref<WebCore::ImageData>>::decode(decoder);

  • trunk/Source/WebKit/Shared/WebCoreArgumentCoders.h

    r276331 r276510  
    11/*
    2  * Copyright (C) 2010-2020 Apple Inc. All rights reserved.
     2 * Copyright (C) 2010-2021 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    705705};
    706706
    707 template<> struct ArgumentCoder<Vector<RefPtr<WebCore::ApplePayError>>> {
    708     static void encode(Encoder&, const Vector<RefPtr<WebCore::ApplePayError>>&);
    709     static Optional<Vector<RefPtr<WebCore::ApplePayError>>> decode(Decoder&);
     707template<> struct ArgumentCoder<RefPtr<WebCore::ApplePayError>> {
     708    static void encode(Encoder&, const RefPtr<WebCore::ApplePayError>&);
     709    static Optional<RefPtr<WebCore::ApplePayError>> decode(Decoder&);
    710710};
    711711
    … …  
    762762};
    763763
    764 template<> struct ArgumentCoder<Vector<RefPtr<WebCore::SecurityOrigin>>> {
    765     static void encode(Encoder&, const Vector<RefPtr<WebCore::SecurityOrigin>>&);
    766     static WARN_UNUSED_RETURN bool decode(Decoder&, Vector<RefPtr<WebCore::SecurityOrigin>>&);
     764template<> struct ArgumentCoder<RefPtr<WebCore::SecurityOrigin>> {
     765    static void encode(Encoder&, const RefPtr<WebCore::SecurityOrigin>&);
     766    static Optional<RefPtr<WebCore::SecurityOrigin>> decode(Decoder&);
    767767};
    768768
    … …  
    811811    static void encode(Encoder&, const WebCore::CDMInstanceSession::Message&);
    812812    static Optional<WebCore::CDMInstanceSession::Message> decode(Decoder&);
    813 };
    814 
    815 template<> struct ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector> {
    816     static void encode(Encoder&, const WebCore::CDMInstanceSession::KeyStatusVector&);
    817     static Optional<WebCore::CDMInstanceSession::KeyStatusVector> decode(Decoder&);
    818813};
    819814#endif
Note: See TracChangeset for help on using the changeset viewer.