Bug#872704: cantata: Dragging local files into playqueue does not work

[Jonas Wielicki]

Hi Stuart,

Thanks for the quick response on a lovely sunday.

On Sonntag, 20. August 2017 21:07:08 CEST you wrote:
> The cantata package in Debian is built with -DENABLE_HTTP_SERVER=OFF because
> exposing services has never seemed like a good idea without a lot of
> careful consideration and detailed code review. 

I see your point.

> I'm quite happy to be
> convinced that this is an appropriate thing to do from a security
> standpoint, but I need convincing.

I think it in fact is. Cantata does a few things to make this secure.

* Requests not from the MPD host are rejected with error 400
  https://github.com/CDrummond/cantata/blob/master/http/httpsocket.cpp#L258

* Only files which have previously been added for HTTP streaming will be 
  opened:

  https://github.com/CDrummond/cantata/blob/master/http/httpsocket.cpp#L282
  https://github.com/CDrummond/cantata/blob/master/http/httpsocket.cpp#L416

From my point of view, this is sufficient. Attack vectors which come into 
mind:

* Bandwidth and memory exhaustion: I’d assume this is already possible via the 
  normal MPD connection (pretending to have an insanely large playqueue for 
  example).

* MPD reading arbitrary files: cantata protects against that by only serving 
  requests for files which have previously been added as streams.

* Anyone else reading streamed files: cantata protects against that by only
  answering requests coming from the MPD host.

  - IP spoofing: the stream is using TCP, so IP spoofing is not entirely
    trivial but still possible in a hostile network.

  I don’t consider this to be a major concern.
  
* Someone under control of the MPD host reading the streamed files: That is
  possible. Requires to know the streamed file name (possibly inferable from
  the played music) and spoofing of the user agent (trivial). I do not 
  consider this a major problem.

* Security issue in the HTTP handling itself: There was a potential DoS/
  memory-exhaustion vector, but the patches I sent upstream for that
  have been applied [1][2][3]. It essentially boiled down to an unfortunate
  default for QTcpSocket::readBufferSize.

  Otherwise, the HTTP handling looks quite sane. There’s no complex parsing
  involved and from what I can tell, there’s (now) no dynamic allocation
  based on peer input or dynamic access to buffers.


I’m not sure if that is convincing. As a last point, I’d like to add that this 
feature makes Cantata pretty amazing, and with the patches applied, I think 
that the attack surface is reduced to a reasonable amount.

What do you think?

kind regards,
Jonas
  

   [1]: https://github.com/CDrummond/cantata/commit/7c9bd03
   [2]: https://github.com/CDrummond/cantata/commit/8fd108c
   [3]: https://github.com/CDrummond/cantata/commit/a9963ad