Merge branch 'develop' into min

README.md

@@ -159,6 +159,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.
 * <strong>v3.0, 2020-02-02, [3.0 beta0(3.0.112)][r3.0b0] released. 121709 lines.</strong>
@@ -1662,6 +1663,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

trunk/src/app/srs_app_config.cpp

@@ -735,7 +735,22 @@ 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);
     }
 

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);

trunk/src/app/srs_app_security.cpp

@@ -42,42 +42,50 @@ srs_error_t SrsSecurity::check(SrsRtmpConnType type, string ip, SrsRequest* req)
 
     // 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);
-    if (!rules) {
+    return do_check(rules, type, ip, req);
-        return err;
 }
 
-    // allow if matches allow strategy.
+srs_error_t SrsSecurity::do_check(SrsConfDirective* rules, SrsRtmpConnType type, string ip, SrsRequest* req)
-    if (allow_check(rules, type, ip) == ERROR_SYSTEM_SECURITY_ALLOW) {
+{
-        srs_error_reset(err);
+    srs_error_t err = srs_success;
+
+    if (!rules) {
+        return srs_error_new(ERROR_SYSTEM_SECURITY, "default deny for %s", ip.c_str());
     }
 
     // deny if matches deny strategy.
-    if (deny_check(rules, type, ip) == ERROR_SYSTEM_SECURITY_DENY) {
+    if ((err = deny_check(rules, type, ip)) != srs_success) {
-        srs_error_reset(err);
+        return srs_error_wrap(err, "for %s", ip.c_str());
-        return srs_error_new(ERROR_SYSTEM_SECURITY_DENY, "denied");
+    }
+    
+    // allow if matches allow strategy.
+    if ((err = allow_check(rules, type, ip)) != srs_success) {
+        return srs_error_wrap(err, "for %s", ip.c_str());
     }
 
     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:
@@ -85,8 +93,7 @@ int SrsSecurity::allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std:
                     break;
                 }
                 if (rule->arg1() == "all" || rule->arg1() == ip) {
-                    ret = ERROR_SYSTEM_SECURITY_ALLOW;
+                    return srs_success; // OK
-                    break;
                 }
                 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;
+                    return srs_success; // OK
-                    break;
                 }
                 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/%d rules", allow_rules, deny_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;
+                    return srs_error_new(ERROR_SYSTEM_SECURITY_DENY, "deny by rule<%s>", rule->arg1().c_str());
-                    break;
                 }
                 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;
+                    return srs_error_new(ERROR_SYSTEM_SECURITY_DENY, "deny by rule<%s>", rule->arg1().c_str());
-                    break;
                 }
                 break;
             case SrsRtmpConnUnknown:
             default:
                 break;
         }
-        
-        // when matched, donot search more.
-        if (ret == ERROR_SYSTEM_SECURITY_DENY) {
-            break;
-        }
     }
     
-    return ret;
+    return srs_success; // OK
 }
 

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,
+    virtual srs_error_t do_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip, SrsRequest* req);
-    // @return, if allowed, ERROR_SYSTEM_SECURITY_ALLOW.
+    virtual srs_error_t allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip);
-    virtual int allow_check(SrsConfDirective* rules, SrsRtmpConnType type, std::string ip);
+    virtual srs_error_t deny_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);
 };
 
 #endif

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

trunk/src/utest/srs_utest_app.cpp

@@ -26,6 +26,8 @@ using namespace std;
 
 #include <srs_kernel_error.hpp>
 #include <srs_app_fragment.hpp>
+#include <srs_app_security.hpp>
+#include <srs_app_config.hpp>
 
 #include <srs_app_st.hpp>
 
@@ -372,3 +374,186 @@ 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));
+    }
+    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) {
+        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));
+    }
+    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) {
+        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));
+    }
+    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) {
+        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));
+    }
+    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) {
+        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.
+}
+