From 369a96a0d93cd4f351957d932443f18f5c43966c Mon Sep 17 00:00:00 2001 From: Ari Chivukula Date: Mon, 22 Aug 2022 17:52:22 +0000 Subject: [PATCH] [ALPS] Add feature gates to parsing code We should have an overall, and a per frame killswitch we can use in finch when needed. This should help address future issues. Bug: 1355247 Change-Id: Idce0c66f3c0754e3268b5111759afc55b1394a6d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3845793 Commit-Queue: David Schinazi Reviewed-by: David Schinazi Auto-Submit: Ari Chivukula Commit-Queue: Ari Chivukula Cr-Commit-Position: refs/heads/main@{#1037842} --- net/base/features.cc | 6 ++++ net/base/features.h | 6 ++++ net/spdy/alps_decoder.cc | 42 ++++++++++++++++++--------- net/spdy/alps_decoder_test.cc | 54 +++++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 14 deletions(-) diff --git a/net/base/features.cc b/net/base/features.cc index a43cca47a57159..539ba96b9524bb 100644 --- a/net/base/features.cc +++ b/net/base/features.cc @@ -344,4 +344,10 @@ const base::FeatureParam kStorageAccessAPIGrantsUnpartitionedStorage( // by the top level site to reduce fingerprinting. const base::Feature kThirdPartyStoragePartitioning{ "ThirdPartyStoragePartitioning", base::FEATURE_DISABLED_BY_DEFAULT}; + +const base::Feature kAlpsParsing{"AlpsParsing", + base::FEATURE_ENABLED_BY_DEFAULT}; + +const base::Feature kAlpsClientHintParsing{"AlpsClientHintParsing", + base::FEATURE_ENABLED_BY_DEFAULT}; } // namespace net::features diff --git a/net/base/features.h b/net/base/features.h index 6729475a4754bf..b73a9aa65ac5ad 100644 --- a/net/base/features.h +++ b/net/base/features.h @@ -487,6 +487,12 @@ NET_EXPORT extern const base::FeatureParam NET_EXPORT extern const base::Feature kThirdPartyStoragePartitioning; +// Whether ALPS parsing is on for any type of frame. +NET_EXPORT extern const base::Feature kAlpsParsing; + +// Whether ALPS parsing is on for client hint parsing specifically. +NET_EXPORT extern const base::Feature kAlpsClientHintParsing; + } // namespace net::features #endif // NET_BASE_FEATURES_H_ diff --git a/net/spdy/alps_decoder.cc b/net/spdy/alps_decoder.cc index 69c8b710387b1a..f07a6670b8c888 100644 --- a/net/spdy/alps_decoder.cc +++ b/net/spdy/alps_decoder.cc @@ -4,6 +4,9 @@ #include "net/spdy/alps_decoder.h" +#include "base/feature_list.h" +#include "net/base/features.h" + namespace net { namespace { @@ -101,23 +104,34 @@ bool AlpsDecoder::AcceptChParser::OnFrameHeader(spdy::SpdyStreamId stream_id, size_t length, uint8_t type, uint8_t flags) { - if (type != static_cast(spdy::SpdyFrameType::ACCEPT_CH) || - error_ != Error::kNoError) { - // Ignore every frame except for ACCEPT_CH. - // Ignore data after an error has occurred. - // Returning false causes Http2DecoderAdapter not to call OnFramePayload(). - return false; - } - if (stream_id != 0) { - error_ = Error::kAcceptChInvalidStream; + // Ignore data after an error has occurred. + if (error_ != Error::kNoError) return false; - } - if (flags != 0) { - error_ = Error::kAcceptChWithFlags; + // Stop all alps parsing if it's disabled. + if (!base::FeatureList::IsEnabled(features::kAlpsParsing)) return false; + // Handle per-type parsing. + switch (type) { + case static_cast(spdy::SpdyFrameType::ACCEPT_CH): { + // Stop alps client hint parsing if it's disabled. + if (!base::FeatureList::IsEnabled(features::kAlpsClientHintParsing)) + return false; + // Check for issues with the frame. + if (stream_id != 0) { + error_ = Error::kAcceptChInvalidStream; + return false; + } + if (flags != 0) { + error_ = Error::kAcceptChWithFlags; + return false; + } + // This frame can be parsed in OnFramePayload. + return true; + } + default: + // Ignore all other types. + return false; } - - return true; } void AlpsDecoder::AcceptChParser::OnFramePayload(const char* data, size_t len) { diff --git a/net/spdy/alps_decoder_test.cc b/net/spdy/alps_decoder_test.cc index 80c98c3a42456e..e2aba3c34ce281 100644 --- a/net/spdy/alps_decoder_test.cc +++ b/net/spdy/alps_decoder_test.cc @@ -4,6 +4,8 @@ #include "net/spdy/alps_decoder.h" +#include "base/test/scoped_feature_list.h" +#include "net/base/features.h" #include "net/base/hex_utils.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -123,6 +125,58 @@ TEST(AlpsDecoderTest, ParseLargeAcceptChFrame) { accept_ch_tokens})); } +TEST(AlpsDecoderTest, DisableAlpsParsing) { + base::test::ScopedFeatureList feature_list; + feature_list.InitAndDisableFeature(features::kAlpsParsing); + AlpsDecoder decoder; + AlpsDecoder::Error error = decoder.Decode(HexDecode( + // ACCEPT_CH frame + "00003d" // length + "89" // type ACCEPT_CH + "00" // flags + "00000000" // stream ID + "0017" // origin length + "68747470733a2f2f7777772e" // + "6578616d706c652e636f6d" // origin "https://www.example.com" + "0003" // value length + "666f6f" // value "foo" + "0018" // origin length + "68747470733a2f2f6d61696c" // + "2e6578616d706c652e636f6d" // origin "https://mail.example.com" + "0003" // value length + "626172" // value "bar" + )); + + EXPECT_EQ(AlpsDecoder::Error::kNoError, error); + EXPECT_THAT(decoder.GetAcceptCh(), IsEmpty()); +} + +TEST(AlpsDecoderTest, DisableAlpsClientHintParsing) { + base::test::ScopedFeatureList feature_list; + feature_list.InitAndDisableFeature(features::kAlpsClientHintParsing); + AlpsDecoder decoder; + AlpsDecoder::Error error = decoder.Decode(HexDecode( + // ACCEPT_CH frame + "00003d" // length + "89" // type ACCEPT_CH + "00" // flags + "00000000" // stream ID + "0017" // origin length + "68747470733a2f2f7777772e" // + "6578616d706c652e636f6d" // origin "https://www.example.com" + "0003" // value length + "666f6f" // value "foo" + "0018" // origin length + "68747470733a2f2f6d61696c" // + "2e6578616d706c652e636f6d" // origin "https://mail.example.com" + "0003" // value length + "626172" // value "bar" + )); + + EXPECT_EQ(AlpsDecoder::Error::kNoError, error); + EXPECT_THAT(decoder.GetAcceptCh(), IsEmpty()); +} + TEST(AlpsDecoderTest, IncompleteFrame) { AlpsDecoder decoder; AlpsDecoder::Error error =