From 8377ba6525de5ebfe33a7dda14f17d96e8ac4ef4 Mon Sep 17 00:00:00 2001 From: "Adam C. Emerson" Date: Mon, 24 Jul 2017 16:10:11 -0400 Subject: [PATCH 1/3] rgw: Fix use after free in IAM policy parser Signed-off-by: Adam C. Emerson --- src/rgw/rgw_iam_policy.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rgw/rgw_iam_policy.cc b/src/rgw/rgw_iam_policy.cc index 9320fd50d77..309b3bab5a3 100644 --- a/src/rgw/rgw_iam_policy.cc +++ b/src/rgw/rgw_iam_policy.cc @@ -688,9 +688,10 @@ bool ParseState::key(const char* s, size_t l) { if (!k) { if (w->kind == TokenKind::cond_op) { + auto id = w->id; auto& t = pp->policy.statements.back(); pp->s.emplace_back(pp, cond_key); - t.conditions.emplace_back(w->id, s, l, cond_ifexists); + t.conditions.emplace_back(id, s, l, cond_ifexists); return true; } else { return false; From 97d026dde679cabf1aaf026be3f08bfef63c140f Mon Sep 17 00:00:00 2001 From: "Adam C. Emerson" Date: Mon, 7 Aug 2017 17:27:53 -0400 Subject: [PATCH 2/3] rgw: Fix another use after free This one was caused by iterator invalidation in set operations. In this case just replace the set entirely with a bitfield. Signed-off-by: Adam C. Emerson --- src/rgw/rgw_iam_policy.cc | 62 +++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/src/rgw/rgw_iam_policy.cc b/src/rgw/rgw_iam_policy.cc index 309b3bab5a3..157b60559c9 100644 --- a/src/rgw/rgw_iam_policy.cc +++ b/src/rgw/rgw_iam_policy.cc @@ -507,7 +507,7 @@ struct PolicyParser : public BaseReaderHandler, PolicyParser> { CephContext* cct; const string& tenant; Policy& policy; - std::set v; + uint32_t v = 0; uint32_t seen = 0; @@ -554,49 +554,59 @@ struct PolicyParser : public BaseReaderHandler, PolicyParser> { } void set(TokenID in) { seen |= dex(in); - if (in == TokenID::Sid || in == TokenID::Effect || in == TokenID::Principal || in == TokenID::NotPrincipal || - in == TokenID::Action || in == TokenID::NotAction || in == TokenID::Resource || in == TokenID::NotResource || - in == TokenID::Condition || in == TokenID::AWS || in == TokenID::Federated || in == TokenID::Service || - in == TokenID::CanonicalUser) { - v.insert(in); + if (dex(in) & (dex(TokenID::Sid) | dex(TokenID::Effect) | + dex(TokenID::Principal) | dex(TokenID::NotPrincipal) | + dex(TokenID::Action) | dex(TokenID::NotAction) | + dex(TokenID::Resource) | dex(TokenID::NotResource) | + dex(TokenID::Condition) | dex(TokenID::AWS) | + dex(TokenID::Federated) | dex(TokenID::Service) | + dex(TokenID::CanonicalUser))) { + v |= dex(in); } } void set(std::initializer_list l) { for (auto in : l) { seen |= dex(in); - if (in == TokenID::Sid || in == TokenID::Effect || in == TokenID::Principal || in == TokenID::NotPrincipal || - in == TokenID::Action || in == TokenID::NotAction || in == TokenID::Resource || in == TokenID::NotResource || - in == TokenID::Condition || in == TokenID::AWS || in == TokenID::Federated || in == TokenID::Service || - in == TokenID::CanonicalUser) { - v.insert(in); + if (dex(in) & (dex(TokenID::Sid) | dex(TokenID::Effect) | + dex(TokenID::Principal) | dex(TokenID::NotPrincipal) | + dex(TokenID::Action) | dex(TokenID::NotAction) | + dex(TokenID::Resource) | dex(TokenID::NotResource) | + dex(TokenID::Condition) | dex(TokenID::AWS) | + dex(TokenID::Federated) | dex(TokenID::Service) | + dex(TokenID::CanonicalUser))) { + v |= dex(in); } } } void reset(TokenID in) { seen &= ~dex(in); - if (in == TokenID::Sid || in == TokenID::Effect || in == TokenID::Principal || in == TokenID::NotPrincipal || - in == TokenID::Action || in == TokenID::NotAction || in == TokenID::Resource || in == TokenID::NotResource || - in == TokenID::Condition || in == TokenID::AWS || in == TokenID::Federated || in == TokenID::Service || - in == TokenID::CanonicalUser) { - v.erase(in); + if (dex(in) & (dex(TokenID::Sid) | dex(TokenID::Effect) | + dex(TokenID::Principal) | dex(TokenID::NotPrincipal) | + dex(TokenID::Action) | dex(TokenID::NotAction) | + dex(TokenID::Resource) | dex(TokenID::NotResource) | + dex(TokenID::Condition) | dex(TokenID::AWS) | + dex(TokenID::Federated) | dex(TokenID::Service) | + dex(TokenID::CanonicalUser))) { + v &= ~dex(in); } } void reset(std::initializer_list l) { for (auto in : l) { seen &= ~dex(in); - if (in == TokenID::Sid || in == TokenID::Effect || in == TokenID::Principal || in == TokenID::NotPrincipal || - in == TokenID::Action || in == TokenID::NotAction || in == TokenID::Resource || in == TokenID::NotResource || - in == TokenID::Condition || in == TokenID::AWS || in == TokenID::Federated || in == TokenID::Service || - in == TokenID::CanonicalUser) { - v.erase(in); + if (dex(in) & (dex(TokenID::Sid) | dex(TokenID::Effect) | + dex(TokenID::Principal) | dex(TokenID::NotPrincipal) | + dex(TokenID::Action) | dex(TokenID::NotAction) | + dex(TokenID::Resource) | dex(TokenID::NotResource) | + dex(TokenID::Condition) | dex(TokenID::AWS) | + dex(TokenID::Federated) | dex(TokenID::Service) | + dex(TokenID::CanonicalUser))) { + v &= ~dex(in); } } } - void reset(std::set v) { - for (auto in : v) { - seen &= ~dex(in); - v.erase(in); - } + void reset(uint32_t& v) { + seen &= ~v; + v = 0; } PolicyParser(CephContext* cct, const string& tenant, Policy& policy) From 5353d952683a5a13a681c594e119b570bfdc3c39 Mon Sep 17 00:00:00 2001 From: "Adam C. Emerson" Date: Mon, 7 Aug 2017 17:46:38 -0400 Subject: [PATCH 3/3] rgw: Fix the last policy use-after-free Signed-off-by: Adam C. Emerson --- src/rgw/rgw_iam_policy.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rgw/rgw_iam_policy.cc b/src/rgw/rgw_iam_policy.cc index 157b60559c9..28b97d04d5c 100644 --- a/src/rgw/rgw_iam_policy.cc +++ b/src/rgw/rgw_iam_policy.cc @@ -700,8 +700,9 @@ bool ParseState::key(const char* s, size_t l) { if (w->kind == TokenKind::cond_op) { auto id = w->id; auto& t = pp->policy.statements.back(); + auto c_ife = cond_ifexists; pp->s.emplace_back(pp, cond_key); - t.conditions.emplace_back(id, s, l, cond_ifexists); + t.conditions.emplace_back(id, s, l, c_ife); return true; } else { return false;