Closed Bug 869725 Opened 11 years ago Closed 11 years ago

support shoutcast streams

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
blocking-b2g -

People

(Reporter: rillian, Assigned: biggyspender)

References

()

Details

Attachments

(1 file, 1 obsolete file)

In bug 862088, it is requested that we support shoutcast mp3-over-http streams in the HTML <audio> tag.

Two known issues:

By default, the shoutcast server sniffs for 'Mozilla' in the user-agent and returns an html index page for the stream rather than the live audio/mpeg data. Not our bug, and this behaviour can be disabled by appending a semicolon (';') to the request url.

The audio element sees the request, even with the semicolon as Content-type: text/plain. As per the HTML spec, we reject that as not an audio stream.
One theory is that shoutcast returns 'ICY 200 OK' which is causing our network stack to return text/plain instead of the supplied content-type. Looking at a packet dump, it does look like the server is sending 'content-type:audio/mpeg' in the response.
I've been poking around at this. Here's what happens:

- http request issued to ShoutCast server with path /;foo.mp3 (including a semi-colon in the path is the only way to solicit mp3 payload from SC)
- SC replies with status line ICY 200 OK followed by HTTP/1.0-like headers
- because this isn't an HTTP status line, nsHttpTransaction::LocateHttpStart fails to locate the status line
- leads to nsHttpResponseHead::ParseVersion assuming HTTP/0.9
- as most streams are on non-default port, nsHttpChannel::CallOnStartRequest forces "text/plain" as mitigation of this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=667907
- as a result of being treated as HTTP/0.9, headers are passed on as part of the content body with content type of "text/plain"
- audio refuses to play due to bad content-type

I've patched my build such that nsHttpTransaction::LocateHttpStart now finds ICY in exactly the same fashion that HTTP/1. is found. nsHttpResponseHead::ParseVersion now also picks up an ICY status and in this case, assumes NS_HTTP_VERSION_1_0 instead of NS_HTTP_VERSION_0_9 . This allows for correct parsing of the subsequent headers, and most importantly, passes on the correct content-type to audio, with a properly aligned content body. With these changes, Shoutcast audio plays correctly and Ralph's example (http://people.mozilla.org/~rgiles/2013/shoutcast.html) shows "Ready to play" and plays perfectly (using a working stream, the one in the example is no longer broadcasting).

I'm completely new to moz development. I don't know if it's wise to approach this in such a way. Given https://bugzilla.mozilla.org/show_bug.cgi?id=667907, there doesn't seem to be much scope for taking this on at a higher level. Anyone care to comment?
Thanks for the analysis. It seems like we have two ways to resolve this bug. We can update the network stack to accept the ICY 200 response, or we can force players to go through the WebAudio api, using XHR and managing the buffering themselves. The later involves CORS restrictions in addition to the added code complexity. Since the whole point with shoutcast support is that the server isn't going to to be updated, that may be a significant additional restriction. How interested are you in being able to play general streams?

If you want to former to happen, you should attach your patch here and try to get it through the review process. It's not obvious to me that accepting 'ICY' as well as 'HTTP/1' introduces significant new XSS risk, but we'll have to have that discussion. The way to make that happen is to suggest a specific code change.
I have good visibility of the adoption of new versions of Shoutcast server and it's extremely slow, with the vast majority of broadcasters still on "SHOUTcast Distributed Network Audio Server/xxxx v1.9.8". Adoption of Shoutcast 2 is just short of 4% after about a year, but I'm not sure that v2 offers any significant change over v1 from the perspective of this issue. 

It has taken 3-4 years for Flash policy file (crossdomain.xml) support to be added, so if I were to start making a noise about adding origin headers, we might expect similar timeframes. I suspect that the ICY header is so baked-in to the Winamp/Shoutcast stack that it will never change, no matter how appealing a standard HTTP status-line might be. In short, the current situation isn't likely to change for a long time.

IMO, the former approach is the only realistic means of adding support. There's already special-casing of a non-standard status line in order to support "SmarterTools/2.0.3974.16813" server ( https://bugzilla.mozilla.org/show_bug.cgi?id=635892 ), so this wouldn't be completely alien.

Once I've made sure I'm up to speed on the submission process, I'll tidy up my changes and attach the patch here.
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch has an overview of the process. I'm happy to help along the way.
I added a secreview flag cc:dveditz to take a look at this; at the very least we can get someone assigned to review it on Wednesday.
Flags: sec-review?(dveditz)
Whiteboard: [triage needed]
Thanks, Yvan. Spender, can you post a provisional patch before then?
Yvan, Ralph... That shouldn't be a problem (qualified with a small amount of uncertainly because this morning my line started suffering from fairly bad packet loss that isn't scheduled to be fixed until Thursday. I suppose I can always fall back on my phone). Thanks.
changing flags and tags so this shows up in Security triage on Wed
Flags: sec-review?(dveditz) → sec-review?
Whiteboard: [triage needed]
Flags: needinfo?(dveditz)
I'm not sure what info is specifically wanted, but from a security POV I'm fine with treating "ICY 200 OK" as synonymous to "HTTP 200 OK" and treating the headers as http 1.0
Flags: needinfo?(dveditz)
Thanks, Daniel. That's the info we wanted. Seemed reasonable to me, but I don't have much experience with XSS.

From the code, I think we'd actually be treating 'ICY' as synonymous with 'HTTP' with the rest of the line parsed as if it was http 1.0. We'll see when spender posts a patch.
Bug 869725 - Changes to HTTP status line detection and parsing to allow AOL/Nullsoft ShoutCast headers (e.g. ICY 200 OK)
Updated my profile with my real name. Apologies for confusion. -spender
Comment on attachment 749459 [details] [diff] [review]
preliminary patch to allow ICY response and treat like HTTP/1.0

Review of attachment 749459 [details] [diff] [review]:
-----------------------------------------------------------------

Some style nits. Otherwise looks reasonable to me.

::: netwerk/protocol/http/nsHttpResponseHead.cpp
@@ +581,5 @@
> +		str += 4;
> +	}
> +	else{
> +		//ICY is HTTP/1.0-like, so below we'll assume HTTP/1.0 because the next char isn't '/'
> +		if (PL_strncasecmp(str, "ICY", 3) == 0) {

Can you make this an:

if (..."HTTP"...) {
  ...
}
else if (...."ICY"...) {
  ...
}
else {
  ...
}

I think that would be easier to read.

@@ +593,2 @@
>      }
> +    

Please remove the trailing whitespace characters here.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +971,5 @@
>  nsHttpTransaction::LocateHttpStart(char *buf, uint32_t len,
>                                     bool aAllowPartialMatch)
>  {
>      NS_ASSERTION(!aAllowPartialMatch || mLineBuf.IsEmpty(), "ouch");
> +	

Whitespace.

@@ +978,5 @@
>      static const char HTTPHeader[] = "HTTP/1.";
>      static const uint32_t HTTPHeaderLen = sizeof(HTTPHeader) - 1;
>      static const char HTTP2Header[] = "HTTP/2.0";
>      static const uint32_t HTTP2HeaderLen = sizeof(HTTP2Header) - 1;
>      

Whitespace.

@@ +1028,5 @@
>              return buf;
>          }
>  
> +		// Treat ICY (AOL/Nullsoft ShoutCast) non-standard header in same 
> +		// fashion as HTTP/2.0 above. This will cause status line to be interpretted 

Please remove trailing whitespace in the comment.
Oops. You don't have to remove trailing whitespace on lines you don't otherwise change. I was just trying to avoid adding more.
When rgiles is happy with the patch don't forget that since it's in /netwerk it needs r+ from actual networking guys before it lands. jduell and mcmanus are good choices or they could suggest others if they're swamped.
Comment on attachment 749459 [details] [diff] [review]
preliminary patch to allow ICY response and treat like HTTP/1.0

Review of attachment 749459 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpResponseHead.cpp
@@ +581,5 @@
> +		str += 4;
> +	}
> +	else{
> +		//ICY is HTTP/1.0-like, so below we'll assume HTTP/1.0 because the next char isn't '/'
> +		if (PL_strncasecmp(str, "ICY", 3) == 0) {

> if (PL_strncasecmp(str, "ICY", 3) == 0) {

What if the line starts ICYFAKE? Since the next char is not '/' this would be accepted. Hm, I guess the old code has the same problem with HTTPNOTREALLY.

If we encounter ICY/1.1 we'll treat it as HTTP/1.1--might be OK but should someone ever create an Icecast x.y server that's different than the corresponding HTTP/x.y we might treat it incorrectly. Probably safest to look for "ICY " and make sure the expected space is there. You can't then drop into common slash-checking code, but if you re-order as suggested by Ralph you won't be doing that anyways.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +978,1 @@
>      static const char HTTPHeader[] = "HTTP/1.";

ObNit: since this is the HTTP protocol it seems odd to lead a section with ICY constants. I'd prefer sticking this on the bottom of the list, and it wouldn't hurt to add a comment saying "// Icecast is HTTP/1.0" or similar. I know you've got the same comment 60 lines down but it can't hurt to leave reminders since this is not expected in normal HTTP code.
Comment on attachment 749459 [details] [diff] [review]
preliminary patch to allow ICY response and treat like HTTP/1.0

Review of attachment 749459 [details] [diff] [review]:
-----------------------------------------------------------------

Chris, thanks for the patch!

This is a totally reasonable thing to do if ICY really is HTTP/1.0. Searching around I didn't find any differences other than the version string, but if there do turn out to be differences in framing or anything like that then we will probably need to find a different approach. But for now, this seems like a good thing to try.

you have a lot of good review comments already, please consider those as part of my comments too.

your patch includes tabs where there should be spaces, please make sure to fix that up. Spaces only.

please assign the r? to me when you're ready.

::: netwerk/protocol/http/nsHttpResponseHead.cpp
@@ +572,2 @@
>  void
>  nsHttpResponseHead::ParseVersion(const char *str)

I would prefer this part of the diff just looks like the below (if it works for you)

diff --git a/netwerk/protocol/http/nsHttpResponseHead.cpp b/netwerk/protocol/http/nsHttpResponseHead.cpp
--- a/netwerk/protocol/http/nsHttpResponseHead.cpp
+++ b/netwerk/protocol/http/nsHttpResponseHead.cpp
@@ -573,16 +573,22 @@ void
 nsHttpResponseHead::ParseVersion(const char *str)
 {
     // Parse HTTP-Version:: "HTTP" "/" 1*DIGIT "." 1*DIGIT
 
     LOG(("nsHttpResponseHead::ParseVersion [version=%s]\n", str));
 
     // make sure we have HTTP at the beginning
     if (PL_strncasecmp(str, "HTTP", 4) != 0) {
+        // Check for ICY (AOL/Nullsoft ShoutCast) and map to 1.0
+        if (!PL_strncasecmp(str, "ICY ", 4)) {
+            LOG(("Treating ICY as HTTP 1.0\n"));
+            mVersion = NS_HTTP_VERSION_1_0;
+            return;
+        }
         LOG(("looks like a HTTP/0.9 response\n"));
         mVersion = NS_HTTP_VERSION_0_9;
         return;
     }
     str += 4;
 
     if (*str != '/') {
         LOG(("server did not send a version number; assuming HTTP/1.0\n"));

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +972,5 @@
>                                     bool aAllowPartialMatch)
>  {
>      NS_ASSERTION(!aAllowPartialMatch || mLineBuf.IsEmpty(), "ouch");
> +	
> +	static const char ICYHeader[] = "ICY";

I think we can make this "ICY " to further target it.
Attachment #749459 - Flags: feedback+
Flags: sec-review? → sec-review?(dveditz)
Removed tabs/trailing spaces/excessive line lengths, and implemented code suggestions and comments.
Attachment #749459 - Attachment is obsolete: true
Attachment #750140 - Flags: review?(mcmanus)
Attachment #750140 - Flags: review?(mcmanus) → review+
Any further action required by me?
(In reply to Chris Sperry from comment #21)
> Any further action required by me?

Not immediately.  I'll push the push to our Try Server now, where we can make sure it builds on all platforms and doesn't break any existing automated tests:
https://tbpl.mozilla.org/?tree=Try&rev=c8a02c2f4df4

Assuming that goes well, one of us with commit access can then land the patch on mozilla-inbound, and within a day or two it will be in Firefox nightly builds!

It would also be good to have an automated test for this bug to make sure it remains fixed.  If that's something you're interested in, you can write a test and attach it as a separate patch on this bug or in a new bug.  The patch in bug 667907 might give you some idea of how to write it; see also:
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js
https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests
The Try run passed, so I landed this on inbound.  Thanks again!
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dab00052bf7
Assignee: nobody → biggyspender
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/0dab00052bf7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Flags: sec-review?(dveditz) → sec-review+
We should backport this patch to b2g18 to enable Shoutcast playback on FirefoxOS 1.1. This will help get Internet radio apps working on FirefoxOS. The patch certainly looks harmless enough.
blocking-b2g: --- → leo?
Triage - new feature, bumping to koi? given current leo stage.
Partner also cannot add/modify feature to the project now as it's near the end of its testing.
blocking-b2g: leo? → koi?
Can we leo? to see if other partners are willing to take this?
blocking-b2g: koi? → leo?
Thanks, everybody, for this fix! Awesome. This just saved us, for an extension we're developing. Thanks a lot.
Filed RFC bug 908902 for the metadata.
(In reply to Chris Lee [:clee] from comment #27)
> Can we leo? to see if other partners are willing to take this?

This is a feature, which it's definitely too late to take at this point for 1.1.
(In reply to Jason Smith [:jsmith] from comment #30)
> (In reply to Chris Lee [:clee] from comment #27)
> > Can we leo? to see if other partners are willing to take this?
> 
> This is a feature, which it's definitely too late to take at this point for
> 1.1.

^ This.

Late features -> late release. If Shoutcast wasn't on the list before this, the priority certainly seems too low to risk at this extremely late point in the release cycle.

It's already landed, which means it's already included in 1.2.
blocking-b2g: leo? → -
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: