URLInputSource can be abused to retrieve arbitrary documents if used naïvely

[alexdutton]

This is mostly related to rdflib-jsonld, but the dereferencing implementation is in rdflib, hence raising it here.

Scenario

If a web service takes POSTed JSON-LD data, e.g. as part of a Linked Data Notifications implementation, rdflib will attempt to resolve any URL in the @context. This can lead to:

• attackers being able to probe internal networks, by having rdflib request potential non-public URLs
• reflection attacks, if the same or slightly-different URLs are repeated multiple times in the @context
• resource exhaustion, as the entire remote file is loaded into memory before JSON parsing is attempted (admittedly an rdflib-jsonld issue)
• denial of service, if web or task workers are tied up waiting for extended periods for HTTP requests to complete
• attackers being able to probe the local filesystem using file:// URLs

Problem

rdflib provides no way to control how external references are resolved, nor a way to implement caching of external resources.

An implementor should be able to:

• add URLs to a safelist, if e.g. they only expect certain JSON-LD contexts to be used
• provide local copies of remote resources, to obviate needing to make HTTP requests
• hook in a caching mechanism

These things should either be possible directly, or there should be an obvious way to hook them in.

Resolution

A new Resolver base class should be added that takes responsibility for resolving external references and returning InputSource instances, probably encapsulating the create_input_source() behaviour in a resolve() method. There should be a default implementation that resolves everything called e.g. DefaultResolver. Maybe this resolver has an instantiation parameter like resolve_schemes=('file', 'http', 'https') so it's easy to turn off dereferencing.

An optional resolver argument should be added to Graph.parse(), so that implementors can override the default behaviour. This is then passed down to the Parser.parse() plugin implementation, defaulting to an instance of DefaultResolver if not specified.

Finally, rdflib-jsonld can be updated to use the resolver instead of create_input_source directly.

Maybe there should also be a way to install a global default resolver to easily implement these protections without having to track down every Graph.parse() call.

Happy to put together a PR if/when an approach is agreed.

[alexdutton]

c.f. https://python-jsonschema.readthedocs.io/en/stable/references/

[nicholascar]

Thanks for calling this issue out @alexdutton. If you plan on addressing it in the way you describe, I'll review it as soon as there's a PR and we can try and merge shortly into a 6.0.x release.

[westurner]

Problem

rdflib provides no way to control how external references are resolved, nor a way to implement caching of external resources.

An implementor should be able to:

• add URLs to a safelist, if e.g. they only expect certain JSON-LD contexts to be used

• JSON-LD Recommended Context
  https://github.com/w3c/json-ld-rc
  https://w3c.github.io/json-ld-rc/context.jsonld

• provide local copies of remote resources, to obviate needing to make HTTP requests

• requests-cache:
  • src: https://pypi.org/project/requests-cache/
  • docs: https://requests-cache.readthedocs.io/en/latest/

  import requests, requests_cache
  requests_cache.install_cache('demo_cache')
  requests.get('http://httpbin.org/delay/1')

• CacheControl
  • src: https://pypi.org/project/CacheControl/
  • docs: https://cachecontrol.readthedocs.io/en/latest/
  • https://github.com/ishitatsuyuki/cachecontrol-sqlite

    import requests
    from cachecontrol import CacheControl
    from cachecontrol_sqlite import SQLiteCache
    
    sess = CacheControl(requests.Session(),
                        cache=SQLiteCache('cache.db'))
                  
    response = sess.get("https://httpbin.org/cache/60")

• hook in a caching mechanism

These things should either be possible directly, or there should be an obvious way to hook them in.

Resolution

A new Resolver base class should be added that takes responsibility for resolving external references and returning InputSource instances, probably encapsulating the create_input_source() behaviour in a resolve() method. There should be a default implementation that resolves everything called e.g. DefaultResolver. Maybe this resolver has an instantiation parameter like resolve_schemes=('file', 'http', 'https') so it's easy to turn off dereferencing.

An optional resolver argument should be added to Graph.parse(), so that implementors can override the default behaviour. This is then passed down to the Parser.parse() plugin implementation, defaulting to an instance of DefaultResolver if not specified.

Finally, rdflib-jsonld can be updated to use the resolver instead of create_input_source directly.

Maybe there should also be a way to install a global default resolver to easily implement these protections without having to track down every Graph.parse() call.

TBH, auto-resolution of contexts maybe should be off by default.

Would a GLOBAL_ENV_VAR like RDFLIB_RESOLVE_EXTERNAL_URIS=1 that defaults to 0 if unset be more safe?

Happy to put together a PR if/when an approach is agreed.

re: the note about Conneg in the source:

• "Schema.org moving from HTTP Content Negotiation to JSON-LD 1.1 "Link:" header for context file"
  Schema.org moving from HTTP Content Negotiation to JSON-LD 1.1 "Link:" header for context file rdflib-jsonld#85

[alexdutton]

Hi @nicholascar :-).

Apologies for the delay, but I've just/finally pushed a draft PR. I've laid it out as a series of logical changes, one per commit, so hopefully it shouldn't be too difficult to follow the rationale for each change in the commit messages.

I've not added any further tests or documentation yet, but am happy to do that once the maintainers have validated the approach.

@westurner I've allow-listed the JSON-LD Recommended Context, but have left caching alone as out of scope (for now). It pays attention to three environment variables (RDFLIB_DEFAULT_RESOLVER_CLASS, RDFLIB_RESOLVABLE_URL_SCHEMES and RDFLIB_RESOLVABLE_URL_ALLOWLIST), which should allow enough basic customisation for people who don't want to change code just yet. WRT conneg, I've made sure that any context found via a Link header uses the same resolution framework.

All of this doesn't change the current graph.parse("https://some/arbitrary/url") behaviour, which will trust whatever URL it's given (but then be circumspect about fetching referenced resources).

[hadasbloom]

@nicholascar Hi, I'm trying to contact you via email regarding this issue (might be going to your spam). If you could take a look there that would be appreciated!
Thanks, Hadas from the Snyk Security Team

[nicholascar]

@hadasbloom apologies, I did see your email but was waiting for work here and, I must admit, I've forgotten about this issue for a while. Apologies. I'll try and follow up on this and have a PR ready from @alexdutton merged shortly and we'll make a new release.

[hadasbloom]

Hey @nicholascar :)
Any updates on when the PR fixing this issue might be merged and released?

[hadasbloom]

Hey, any updates on this issue?
I't been open since July and we will have to go ahead and publish it according to our 90-day disclosure policy.
@nicholascar I'd be glad to chat about more specific details privately via email, and we can wait a bit more if a fix is planned :)

[aucampia]

I have locked this disucssion, please take further discussion on #1844 - this should be an issue, not a discussion topic.