From c51c378869747d40d4bd799be37decedc0c931f4 Mon Sep 17 00:00:00 2001 From: winlin Date: Tue, 4 Feb 2020 19:07:54 +0800 Subject: [PATCH 1/3] For #1186, refactor security check. 3.0.114 --- README.md | 2 + trunk/src/app/srs_app_config.cpp | 17 +++- trunk/src/app/srs_app_config.hpp | 1 + trunk/src/app/srs_app_security.cpp | 85 ++++++++++---------- trunk/src/app/srs_app_security.hpp | 9 +-- trunk/src/core/srs_core_version3.hpp | 2 +- trunk/src/utest/srs_utest_app.cpp | 115 +++++++++++++++++++++++++++ 7 files changed, 178 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index ecdc0fad3..ead8a55d2 100755 --- a/README.md +++ b/README.md @@ -146,6 +146,7 @@ For previous versions, please read: ## V3 changes +* v3.0, 2020-02-04, For [#1186][bug #1186], refactor security check. 3.0.114 * v3.0, 2020-02-04, Fix [#939][bug #939], response right A/V flag in FLV header. 3.0.113 * v3.0, 2020-02-04, For [#939][bug #939], always enable fast FLV streaming. * v3.0, 2020-02-02, [3.0 beta0(3.0.112)][r3.0b0] released. 121709 lines. @@ -1640,6 +1641,7 @@ Winlin [bug #1230]: https://github.com/ossrs/srs/issues/1230 [bug #1206]: https://github.com/ossrs/srs/issues/1206 [bug #939]: https://github.com/ossrs/srs/issues/939 +[bug #1186]: https://github.com/ossrs/srs/issues/1186 [bug #xxxxxxxxxxxxx]: https://github.com/ossrs/srs/issues/xxxxxxxxxxxxx [exo #828]: https://github.com/google/ExoPlayer/pull/828 diff --git a/trunk/src/app/srs_app_config.cpp b/trunk/src/app/srs_app_config.cpp index 20594b917..b75d0ac81 100644 --- a/trunk/src/app/srs_app_config.cpp +++ b/trunk/src/app/srs_app_config.cpp @@ -735,13 +735,28 @@ SrsConfDirective* SrsConfDirective::get_or_create(string n, string a0) if (!conf) { conf = new SrsConfDirective(); conf->name = n; - conf->set_arg0(a0); + conf->args.push_back(a0); directives.push_back(conf); } return conf; } +SrsConfDirective* SrsConfDirective::get_or_create(string n, string a0, string a1) +{ + SrsConfDirective* conf = get(n, a0); + + if (!conf) { + conf = new SrsConfDirective(); + conf->name = n; + conf->args.push_back(a0); + conf->args.push_back(a1); + directives.push_back(conf); + } + + return conf; +} + SrsConfDirective* SrsConfDirective::set_arg0(string a0) { if (arg0() == a0) { diff --git a/trunk/src/app/srs_app_config.hpp b/trunk/src/app/srs_app_config.hpp index c9c03e24f..03ed3e858 100644 --- a/trunk/src/app/srs_app_config.hpp +++ b/trunk/src/app/srs_app_config.hpp @@ -215,6 +215,7 @@ public: public: virtual SrsConfDirective* get_or_create(std::string n); virtual SrsConfDirective* get_or_create(std::string n, std::string a0); + virtual SrsConfDirective* get_or_create(std::string n, std::string a0, std::string a1); virtual SrsConfDirective* set_arg0(std::string a0); // Remove the v from sub directives, user must free the v. virtual void remove(SrsConfDirective* v); diff --git a/trunk/src/app/srs_app_security.cpp b/trunk/src/app/srs_app_security.cpp index 06e886fc7..aabad0800 100644 --- a/trunk/src/app/srs_app_security.cpp +++ b/trunk/src/app/srs_app_security.cpp @@ -39,54 +39,61 @@ SrsSecurity::~SrsSecurity() srs_error_t SrsSecurity::check(SrsRtmpConnType type, string ip, SrsRequest* req) { srs_error_t err = srs_success; - + // allow all if security disabled. if (!_srs_config->get_security_enabled(req->vhost)) { - return err; + return err; // OK } - - // default to deny all when security enabled. - err = srs_error_new(ERROR_SYSTEM_SECURITY, "allowed"); - + // rules to apply SrsConfDirective* rules = _srs_config->get_security_rules(req->vhost); + return do_check(rules, type, ip, req); +} + +srs_error_t SrsSecurity::do_check(SrsConfDirective* rules, SrsRtmpConnType type, string ip, SrsRequest* req) +{ + srs_error_t err = srs_success; + if (!rules) { - return err; + return srs_error_new(ERROR_SYSTEM_SECURITY, "default deny for %s", ip.c_str()); + } + + // deny if matches deny strategy. + if ((err = deny_check(rules, type, ip)) != srs_success) { + return srs_error_wrap(err, "for %s", ip.c_str()); } // allow if matches allow strategy. - if (allow_check(rules, type, ip) == ERROR_SYSTEM_SECURITY_ALLOW) { - srs_error_reset(err); + if ((err = allow_check(rules, type, ip)) != srs_success) { + return srs_error_wrap(err, "for %s", ip.c_str()); } - - // deny if matches deny strategy. - if (deny_check(rules, type, ip) == ERROR_SYSTEM_SECURITY_DENY) { - srs_error_reset(err); - return srs_error_new(ERROR_SYSTEM_SECURITY_DENY, "denied"); - } - + return err; } -int SrsSecurity::allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip) +srs_error_t SrsSecurity::allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip) { - int ret = ERROR_SUCCESS; - + int allow_rules = 0; + int deny_rules = 0; + for (int i = 0; i < (int)rules->directives.size(); i++) { SrsConfDirective* rule = rules->at(i); - + if (rule->name != "allow") { + if (rule->name == "deny") { + deny_rules++; + } continue; } - + allow_rules++; + switch (type) { case SrsRtmpConnPlay: if (rule->arg0() != "play") { break; } if (rule->arg1() == "all" || rule->arg1() == ip) { - ret = ERROR_SYSTEM_SECURITY_ALLOW; - break; + return srs_success; // OK } break; case SrsRtmpConnFMLEPublish: @@ -96,28 +103,23 @@ int SrsSecurity::allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std: break; } if (rule->arg1() == "all" || rule->arg1() == ip) { - ret = ERROR_SYSTEM_SECURITY_ALLOW; - break; + return srs_success; // OK } break; case SrsRtmpConnUnknown: default: break; } - - // when matched, donot search more. - if (ret == ERROR_SYSTEM_SECURITY_ALLOW) { - break; - } } - - return ret; + + if (allow_rules > 0 || (deny_rules + allow_rules) == 0) { + return srs_error_new(ERROR_SYSTEM_SECURITY_ALLOW, "not allowed by any of %d rules", allow_rules); + } + return srs_success; // OK } -int SrsSecurity::deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip) +srs_error_t SrsSecurity::deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip) { - int ret = ERROR_SUCCESS; - for (int i = 0; i < (int)rules->directives.size(); i++) { SrsConfDirective* rule = rules->at(i); @@ -131,8 +133,7 @@ int SrsSecurity::deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std:: break; } if (rule->arg1() == "all" || rule->arg1() == ip) { - ret = ERROR_SYSTEM_SECURITY_DENY; - break; + return srs_error_new(ERROR_SYSTEM_SECURITY_DENY, "deny by rule<%s>", rule->arg1().c_str()); } break; case SrsRtmpConnFMLEPublish: @@ -142,21 +143,15 @@ int SrsSecurity::deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std:: break; } if (rule->arg1() == "all" || rule->arg1() == ip) { - ret = ERROR_SYSTEM_SECURITY_DENY; - break; + return srs_error_new(ERROR_SYSTEM_SECURITY_DENY, "deny by rule<%s>", rule->arg1().c_str()); } break; case SrsRtmpConnUnknown: default: break; } - - // when matched, donot search more. - if (ret == ERROR_SYSTEM_SECURITY_DENY) { - break; - } } - return ret; + return srs_success; // OK } diff --git a/trunk/src/app/srs_app_security.hpp b/trunk/src/app/srs_app_security.hpp index e336f1a90..163b7d3a6 100644 --- a/trunk/src/app/srs_app_security.hpp +++ b/trunk/src/app/srs_app_security.hpp @@ -46,12 +46,9 @@ public: // @param req the request object of client. virtual srs_error_t check(SrsRtmpConnType type, std::string ip, SrsRequest* req); private: - // Security check the allow, - // @return, if allowed, ERROR_SYSTEM_SECURITY_ALLOW. - virtual int allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip); - // Security check the deny, - // @return, if denied, ERROR_SYSTEM_SECURITY_DENY. - virtual int deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip); + virtual srs_error_t do_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip, SrsRequest* req); + virtual srs_error_t allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip); + virtual srs_error_t deny_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip); }; #endif diff --git a/trunk/src/core/srs_core_version3.hpp b/trunk/src/core/srs_core_version3.hpp index 0977c5458..fe5e458d1 100644 --- a/trunk/src/core/srs_core_version3.hpp +++ b/trunk/src/core/srs_core_version3.hpp @@ -24,6 +24,6 @@ #ifndef SRS_CORE_VERSION3_HPP #define SRS_CORE_VERSION3_HPP -#define SRS_VERSION3_REVISION 113 +#define SRS_VERSION3_REVISION 114 #endif diff --git a/trunk/src/utest/srs_utest_app.cpp b/trunk/src/utest/srs_utest_app.cpp index 5d158d5d6..c7f0894e1 100644 --- a/trunk/src/utest/srs_utest_app.cpp +++ b/trunk/src/utest/srs_utest_app.cpp @@ -26,6 +26,8 @@ using namespace std; #include #include +#include +#include #include @@ -372,3 +374,116 @@ VOID TEST(AppFragmentTest, CheckDuration) } } +VOID TEST(AppSecurity, CheckSecurity) +{ + srs_error_t err; + + // Deny if no rules. + if (true) { + SrsSecurity sec; SrsRequest rr; + HELPER_EXPECT_FAILED(sec.do_check(NULL, SrsRtmpConnUnknown, "", &rr)); + } + + // Deny if not allowed. + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnUnknown, "", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("others"); rules.get_or_create("any"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnUnknown, "", &rr)); + } + + // Deny by rule. + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "play", "all"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "play", "12.13.14.15"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "play", "11.12.13.14"); + if (true) { + SrsConfDirective* d = new SrsConfDirective(); + d->name = "deny"; + d->args.push_back("play"); + d->args.push_back("12.13.14.15"); + rules.directives.push_back(d); + } + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); + } + + // Allowed if not denied. + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "play", "all"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "play", "12.13.14.15"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "play", "12.13.14.15"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "11.12.13.14", &rr)); + } + + // Allowed by rule. + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "play", "12.13.14.15"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "play", "all"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); + } + + // Allowed if not denied. + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "play", "12.13.14.15"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "play", "all"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr)); + } + + // Denied if not allowd. + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "play", "11.12.13.14"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "play", "11.12.13.14"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); + } + + // Denied if dup. + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "play", "11.12.13.14"); + rules.get_or_create("deny", "play", "11.12.13.14"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "11.12.13.14", &rr)); + } + + // SRS apply the following simple strategies one by one: + // 1. allow all if security disabled. + // 2. default to deny all when security enabled. + // 3. allow if matches allow strategy. + // 4. deny if matches deny strategy. +} + From b9d45ba7d9cb3e3e4f79ba714d693568e5312699 Mon Sep 17 00:00:00 2001 From: winlin Date: Tue, 4 Feb 2020 19:21:15 +0800 Subject: [PATCH 2/3] For #1186, refactor security check. 3.0.114 --- trunk/src/utest/srs_utest_app.cpp | 70 +++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/trunk/src/utest/srs_utest_app.cpp b/trunk/src/utest/srs_utest_app.cpp index c7f0894e1..98c34a29a 100644 --- a/trunk/src/utest/srs_utest_app.cpp +++ b/trunk/src/utest/srs_utest_app.cpp @@ -418,6 +418,26 @@ VOID TEST(AppSecurity, CheckSecurity) } HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "publish", "12.13.14.15"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "publish", "12.13.14.15"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnFlashPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "publish", "all"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnFlashPublish, "11.12.13.14", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "publish", "12.13.14.15"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnHaivisionPublish, "12.13.14.15", &rr)); + } // Allowed if not denied. if (true) { @@ -435,6 +455,21 @@ VOID TEST(AppSecurity, CheckSecurity) rules.get_or_create("deny", "play", "12.13.14.15"); HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "11.12.13.14", &rr)); } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "publish", "12.13.14.15"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "publish", "12.13.14.15"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnUnknown, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("deny", "publish", "12.13.14.15"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFlashPublish, "11.12.13.14", &rr)); + } // Allowed by rule. if (true) { @@ -447,6 +482,26 @@ VOID TEST(AppSecurity, CheckSecurity) rules.get_or_create("allow", "play", "all"); HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "publish", "all"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFMLEPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "publish", "all"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnFlashPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "publish", "all"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnHaivisionPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "publish", "12.13.14.15"); + HELPER_EXPECT_SUCCESS(sec.do_check(&rules, SrsRtmpConnHaivisionPublish, "12.13.14.15", &rr)); + } // Allowed if not denied. if (true) { @@ -471,6 +526,21 @@ VOID TEST(AppSecurity, CheckSecurity) rules.get_or_create("allow", "play", "11.12.13.14"); HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "publish", "12.13.14.15"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnPlay, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "publish", "11.12.13.14"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnHaivisionPublish, "12.13.14.15", &rr)); + } + if (true) { + SrsSecurity sec; SrsRequest rr; SrsConfDirective rules; + rules.get_or_create("allow", "publish", "11.12.13.14"); + HELPER_EXPECT_FAILED(sec.do_check(&rules, SrsRtmpConnUnknown, "11.12.13.14", &rr)); + } // Denied if dup. if (true) { From a99cee28193ba7da5a098e66841633592684c718 Mon Sep 17 00:00:00 2001 From: winlin Date: Tue, 4 Feb 2020 19:33:11 +0800 Subject: [PATCH 3/3] For #1186, refactor security check. 3.0.114 --- trunk/src/app/srs_app_security.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/trunk/src/app/srs_app_security.cpp b/trunk/src/app/srs_app_security.cpp index aabad0800..812766f08 100644 --- a/trunk/src/app/srs_app_security.cpp +++ b/trunk/src/app/srs_app_security.cpp @@ -113,7 +113,7 @@ srs_error_t SrsSecurity::allow_check(SrsConfDirective* rules, SrsRtmpConnType ty } if (allow_rules > 0 || (deny_rules + allow_rules) == 0) { - return srs_error_new(ERROR_SYSTEM_SECURITY_ALLOW, "not allowed by any of %d rules", allow_rules); + return srs_error_new(ERROR_SYSTEM_SECURITY_ALLOW, "not allowed by any of %d/%d rules", allow_rules, deny_rules); } return srs_success; // OK }