adlnet / xAPI-Spec

The xAPI Specification describes communication about learner activity and experiences between technologies.
https://adlnet.gov/projects/xapi/
901 stars 405 forks source link

Alternate Request Syntax document apis security issue. #1017

Open garemoko opened 7 years ago

garemoko commented 7 years ago

See: https://github.com/adlnet/xAPI-Spec/blob/master/xAPI-Communication.md#alt-request-syntax And: https://groups.google.com/a/adlnet.gov/forum/#!topic/xapi-spec/mMNBNMyrcnI

An attacker with activity provider credentials could store an HTML page in the document APIs and then trick a user logged in to an LRS into visiting the page. As the page is hosted by the LRS, it could potentially access the LRS user's cookies. This is a particular risk for alternate request syntax because the request does not require any special headers that a browser would not normally send.

andyjohnson commented 7 years ago

Idea on the Dec 21, 2016 call was to document this as a conformance issue. For patch and in testing documentation - "HTML will not be tested as a part of Conformance Testing and suggest that HTML is not counted on for interoperability." For 2.0, Consider suggesting that HTML returned would return plain text or not allowing HTML in the first place. Clients SHOULD NOT* store HTML :)

garemoko commented 7 years ago

For next patch version: note the security concern and suggest that LRSs return the document as plain text.

For 2.0 LRS may reject documents of content type HTML

bscSCORM commented 7 years ago

@garemoko @andyjohnson after we got off the call, I realized that this only really need apply to the alternate request version. I'm not it's worth making the distinction, but it seems like an LRS could address this security concern by re-writing the content type only when HTML otherwise would be returned via the alternate request syntax

garemoko commented 7 years ago

@bscSCORM good point, yes.

brianjmiller commented 7 years ago

FWIW, for a 2.x version I'd advocate dropping the alternate request syntax completely.

bscSCORM commented 7 years ago

@brianjmiller do you expect LRSs that want to support the alternate syntax will just continue to support 1.0, or rather just figure that the syntax is no longer needed?

brianjmiller commented 7 years ago

I can't tell if you are asking me whether I think the syntax is no longer needed or whether the LRS thinks the syntax is no longer needed.

I was saying I don't think the syntax is still needed. I'd expect LRSs wanting to support 1.0 to have it for conformance, but I'd lower the barrier to conformance for LRSs supporting 2.0+. AFAICT by the time we'd get to a 2.x (which I'm assuming isn't soon) then the only reason to have it would be URL length and I just don't see that as a problem that needs to be handled at the spec level.

rmlowe commented 7 years ago

So I wasn't on the call, but the attack described in the thread linked from the first comment uses CSRF, so it works equally well if the HTML form is hosted on a third-party site. That is, it does not depend on the LRS producing or consuming HTML, via Document Resources or otherwise. I don't believe the suggestions above do anything to address that.

There is certainly also a potential attack whereby a malicious activity provider uses Document Resources to inject scripts into the LRS origin, possibly aided by the alternate syntax. However I think it's important to understand that that's a different attack, with quite different mitigations.

garemoko commented 7 years ago

@rmlowe thanks for clarifying that. I was conflating the two attacks when I created this issue so edited it yesterday to try and clarify that it's the second issue ('a malicious activity provider uses Document Resources to inject scripts into the LRS origin') that this thread is concerned about.