[PATCH v2] avformat/rtmpproto: add rtmp_strict_paths option

From: Will Martin <[email protected]>

In a multi-tenant RTMP ingest that assigns one listening port per
customer, ports are necessarily reused as customers come and go. A
customer that has been moved to a different port, but whose encoder is
still configured with the old one, will reconnect to a port that now
belongs to somebody else. Today the server logs a warning about the
mismatched application and stream name and then ingests the stream
anyway, so one tenant's video ends up in another tenant's pipeline.

The application and stream name in the URL are the only per-tenant
discriminator available at the protocol level, so make it possible to
enforce them. With -rtmp_strict_paths 1, a mismatch of the connect
"app" field or of the published stream name against the listening URL
fails the connection instead of only logging a warning. The default
behavior is unchanged.

This is a filter on which peers an ingest accepts, not authentication:
the path is transmitted in clear by plain RTMP and offers no secrecy
against an attacker who can observe or guess it. It is intended to stop
stale and misconfigured clients from landing on the wrong tenant.

The option is protocol level, so the existing -strict /
strict_std_compliance cannot be used for it: URLContext has no
compliance field, and strict_std_compliance describes specification
conformance rather than which peers an ingest is willing to accept.

Signed-off-by: Will Martin <[email protected]>
[renato.cron: rebased on master; dropped the librtmp.c hunk; fixed the
 packet leak in read_connect(); added documentation; reworded the log
 messages]
Signed-off-by: Renato CRON <[email protected]>
---

Resubmission of a patch from 2019 that never got applied:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/[email protected]/

I hit exactly the problem it describes in production and would like to
revive it. Authorship is left with Will Martin since the design and most
of the code are theirs; I have kept them in CC in case they object to
this being carried forward. Rebased onto current master and updated to
address the review comments the original submission received:

 - Michael: the option is now documented in doc/protocols.texi.

 - Reino: "App field don't match up" -> "doesn't". The pre-existing
   warning is corrected too so both messages read the same.

 - Carl Eugen: I kept a dedicated option instead of reusing -strict.
   strict_std_compliance lives on AVCodecContext/AVFormatContext, while
   this check runs in a URLProtocol, and URLContext has no compliance
   field to consult. The meaning also differs: strict_std_compliance is
   about conforming to a specification, not about which peers an ingest
   is willing to accept. I am happy to change this if you disagree.

Two errors in the original patch are fixed as well:

 - The libavformat/librtmp.c hunk is dropped entirely. It declared the
   field as bool without including <stdbool.h>, which does not compile
   under -std=c17, and it declared an AV_OPT_TYPE_BOOL option against a
   bool field, which av_opt_set_int would write to as an int. It was
   dead weight regardless: librtmp.c implements no listen mode, so
   nothing there could ever read the flag.

 - The new error path in read_connect() returned without calling
   ff_rtmp_packet_destroy(), leaking the connect packet.

Testing, ffmpeg acting as both listener and publisher over loopback:

  strict=1, stream name mismatch -> connection rejected, EIO
  strict=1, app name mismatch    -> connection rejected, EIO
  strict=1, everything matches   -> stream received normally
  strict=0, stream name mismatch -> warning only, stream received,
                                    i.e. current behavior preserved

The leak fix was confirmed with a gcc-asan build: the app-mismatch case
reports "Direct leak of 141 byte(s) in 1 object(s)" without the
ff_rtmp_packet_destroy() call and is clean with it.

One limitation worth stating plainly: this does not make -listen 1 a
persistent server. ff_listen_bind() accepts a single connection and then
closes the listening socket, so a rejected client ends the process
rather than freeing the port for the next one. That is pre-existing
behavior and not something this option changes, but it does mean a
stale client that reconnects in a loop can keep a supervised ingest
flapping. I still consider refusing the stream better than ingesting it
into the wrong tenant.
 doc/protocols.texi      |  7 +++++++
 libavformat/rtmpproto.c | 23 ++++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/doc/protocols.texi b/doc/protocols.texi
index f7e65b1c9d..0f9cceb19f 100644
--- a/doc/protocols.texi
+++ b/doc/protocols.texi
@@ -977,6 +977,13 @@ value will be sent.
 Stream identifier to play or to publish. This option overrides the
 parameter specified in the URI.

+@item rtmp_strict_paths=@var{1|0}
+When listening for incoming connections, reject a connection whose
+application name or stream name does not match the one given in the
+listening URL, instead of only logging a warning and accepting it.
+Useful when several RTMP ingest points share the same port. Default
+value is 0.
+
 @item rtmp_subscribe
 Name of live stream to subscribe to. By default no value will be sent.
 It is only sent if the option is specified or if rtmp_live
diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
index 785455e06d..74ca6bd016 100644
--- a/libavformat/rtmpproto.c
+++ b/libavformat/rtmpproto.c
@@ -134,6 +134,7 @@ typedef struct RTMPContext {
     char          auth_params[500];
     int           do_reconnect;
     int           auth_tried;
+    int           strict_paths;
 } RTMPContext;

 #define PLAYER_KEY_OPEN_PART_LEN 30   ///< length of partial key used for first client digest signing
@@ -540,9 +541,17 @@ static int read_connect(URLContext *s, RTMPContext *rt)
                                  "app", tmpstr, sizeof(tmpstr));
     if (ret)
         av_log(s, AV_LOG_WARNING, "App field not found in connect\n");
-    if (!ret && strcmp(tmpstr, rt->app))
-        av_log(s, AV_LOG_WARNING, "App field don't match up: %s <-> %s\n",
+    if (!ret && strcmp(tmpstr, rt->app)) {
+        if (rt->strict_paths) {
+            av_log(s, AV_LOG_ERROR, "App field doesn't match up: %s <-> %s, "
+                   "rejecting connection since rtmp_strict_paths is set\n",
+                   tmpstr, rt->app);
+            ff_rtmp_packet_destroy(&pkt);
+            return AVERROR(EIO);
+        }
+        av_log(s, AV_LOG_WARNING, "App field doesn't match up: %s <-> %s\n",
                tmpstr, rt->app);
+    }
     ff_rtmp_packet_destroy(&pkt);

     // Send Window Acknowledgement Size (as defined in specification)
@@ -2029,9 +2038,16 @@ static int send_invoke_response(URLContext *s, RTMPPacket *pkt)
                 pchar = s->filename;
             }
             pchar++;
-            if (strcmp(pchar, filename))
+            if (strcmp(pchar, filename)) {
+                if (rt->strict_paths) {
+                    av_log(s, AV_LOG_ERROR, "Unexpected stream %s, expecting %s, "
+                           "rejecting publish since rtmp_strict_paths is set\n",
+                           filename, pchar);
+                    return AVERROR(EIO);
+                }
                 av_log(s, AV_LOG_WARNING, "Unexpected stream %s, expecting"
                        " %s\n", filename, pchar);
+            }
         }
         rt->state = STATE_RECEIVING;
     }
@@ -3215,6 +3231,7 @@ static const AVOption rtmp_options[] = {
     {"recorded", "recorded stream", 0, AV_OPT_TYPE_CONST, {.i64 = 0}, 0, 0, DEC, .unit = "rtmp_live"},
     {"rtmp_pageurl", "URL of the web page in which the media was embedded. By default no value will be sent.", OFFSET(pageurl), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC},
     {"rtmp_playpath", "Stream identifier to play or to publish", OFFSET(playpath), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC|ENC},
+    {"rtmp_strict_paths", "Error instead of warn for mismatch on stream or application path in url", OFFSET(strict_paths), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, DEC},
     {"rtmp_subscribe", "Name of live stream to subscribe to. Defaults to rtmp_playpath.", OFFSET(subscribe), AV_OPT_TYPE_STRING, {.str = NULL }, 0, 0, DEC},
     {"rtmp_swfhash", "SHA256 hash of the decompressed SWF file (32 bytes).", OFFSET(swfhash), AV_OPT_TYPE_BINARY, .flags = DEC},
     {"rtmp_swfsize", "Size of the decompressed SWF file, required for SWFVerification.", OFFSET(swfsize), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, DEC},
--
2.34.1

_______________________________________________
ffmpeg-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]