1212239 - Disallow directory picking for non-"secure context" pages (to encourage HTTPS uploads)

Status: NEW

(Core :: DOM: Core & HTML, task)

Description

[Jonathan Watt [:jwatt]]

Spinning off from bug 907707.

One of the concerns raised during discussions of directory picking with the security team is the concern that a user may unwittingly pick a directory containing files that they don't want to give the website access to. This could happen if the user forgets that the directory contains those files, or because they are unaware of the sensitive nature of information that the operating system or their applications store there. Unlike with file picking, a user isn't making a direct selection of individual files.

To mitigate the fallout when this happens to a user it would be preferable to force websites to upload files from a directory pick via HTTPS. That way at least the user's sensitive files should only have been exposed to the website, rather than sniffed by a third party.

Unfortunately it isn't really practical to enforce that *uploads* from a directory pick are via HTTPS, since the contents of files can be converted to strings via FileReader allowing content authors to easily work around such restrictions. (We could in principle have a string tainting mechanism and add code to propagate the taint and prevent use of tainted strings in loads of places, but that would require very extensive changes, and they would have a performance cost in loads of unrelated code. That isn't practical or proportionate.)

What we can do to at least significantly increase the chances of uploads being made via HTTPS is to restrict directory picking to pages that have been loaded via HTTPS. If a website is already using HTTPS it is likely that they will upload files via HTTPS too.

The imminent release of https://letsencrypt.org/ makes an HTTPS restriction a more realistic restriction to impose.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch to disallow directory picking for HTTP

Comment 2

[(no longer active)]

Are you not worried about the case of an https iframe in an http page?  See <https://w3c.github.io/webappsec/specs/powerfulfeatures/>.

(I should also say that I don't really agree with this decision, but if we're going to do it, we should do it properly I think.)

Comment 3

[Jonathan Watt [:jwatt]]

I just had a chat with Ehsan on IRC. To expand on the https-iframe-in-http comment, the very valid concern here is that an http page could include an https page that provides it with the functionality to bring up a directory picker. For example, someone could host a library on github and widely publicize it as a method of working around any https restriction. So yes, for there to be any point in the https restriction, we would more specifically need to restrict directory picking to "secure contexts".

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8670664 [details] [diff] [review]
patch to disallow directory picking for HTTP

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

::: dom/html/HTMLInputElement.cpp
@@ +866,5 @@
>    , mNumberControlSpinnerSpinsUp(false)
>    , mPickerRunning(false)
>    , mSelectionCached(true)
> +  , mCheckedDirectoryPickerIsAllowed(false)
> +  , mDirectoryPickerIsAllowed(false)

Instead having 2 booleans, what about:

enum {
  Unknown,
  Deny,
  Allowed
} mDirectoryPickerIsAllowed;

, mDirectoryPickerIsAllowed(Unknown)

@@ +4835,5 @@
>  // Directory picking methods:
>  
>  bool
> +HTMLInputElement::IsDirectoryPickerEnabled() const
> +{

If you like the enum, this would be:

if (mIsDirectoryPickerAllowed == Unknown) {
  return mIsDirectoryPickerAllowed == Allowed;
}

// Just to have an easy 'return false' approach.
mIsDirectoryPickerAllowed = Deny;

if (nsContentUtils::IsChromeDoc(...
  mIsDirectoryPickerAllowed = Allowed;
  return true;
}

...

@@ +4841,5 @@
> +    if (nsContentUtils::IsChromeDoc(OwnerDoc())) {
> +      mDirectoryPickerIsAllowed = true;
> +    } else {
> +      nsIPrincipal* principal = NodePrincipal();
> +      if (!principal) {

MOZ_ASSERT(principal); This cannot be null.

@@ +4846,5 @@
> +        return false;
> +      }
> +      nsCOMPtr<nsIURI> principalURI;
> +      nsresult rv = principal->GetURI(getter_AddRefs(principalURI));
> +      if (NS_FAILED(rv)) {

1. NS_WARN_IF
2. We should still set mCheckedDirectoryPickerIsAllowed to true, correct?

@@ +4850,5 @@
> +      if (NS_FAILED(rv)) {
> +        return false;
> +      }
> +      if (!principalURI) {
> +        principalURI = OwnerDoc()->GetDocumentURI();

this can be null again.

@@ +4854,5 @@
> +        principalURI = OwnerDoc()->GetDocumentURI();
> +      }
> +      bool isHTTP;
> +      rv = principalURI->SchemeIs("http", &isHTTP);
> +      if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +4884,5 @@
>      return;
>    }
> +  if (!IsDirectoryPickerEnabled()) {
> +    // Send message about HTTPS being required using
> +    // nsContentUtils::ReportToConsoleNonLocalized/ReportToConsole?

This would be nice. Yes.

::: dom/html/HTMLInputElement.h
@@ +705,5 @@
> +   * This will return false if the page was loaded via HTTP since we want to
> +   * encourage content authors to use HTTPS. This is largely to reduce the
> +   * potential fallout of a user uploading a directory that contains sensitive
> +   * files that they did not intent to upload.
> +   * 

extra space.

Comment 5

[Jesse Ruderman]

Do you plan to redo this patch to use the infrastructure from bug 1162772?

Comment 6

[Jonathan Watt [:jwatt]]

That has been my plan, but there are still a bunch of open questions about the "secure context" stuff; the patches over there are only up to provide a basis for discussion for now. Figuring out how things are supposed to behave involves following a bit of a rats nest of spec text and algorithms (spread over multiple whatwg specs), there are a bunch of open questions about the behavior, and then there's the question of whether what the specs define is desirable behavior for us here. If I can't really get that stuff nailed down soon then I may just split out the code for a best effort pass for now.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

This depends on us implementing bug 1162772.

Comment 8

[Jonathan Watt [:jwatt]]

I went with two booleans just now since it keeps the size of HTMLInputElement from bloating any more that in already has.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8686124 [details] [diff] [review]
patch

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

::: dom/html/HTMLInputElement.cpp
@@ +4847,5 @@
> +        return false;
> +      }
> +      if (window->IsSecureContext()) {
> +        mDirectoryPickerIsAllowed = true;
> +        const char16_t* strings[] = { NS_LITERAL_CSTRING("directory picking") };

I would put this NS_LITERAL_CSTRING outside, otherwise it can be deleted when it goes out of scope, correct?
Plus, you want a char16_t*, and that should be a NS_LITERAL_STRING().

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: patch to mark the directory picker WebIDL API as [SecureContext]

Comment 11

[Jonathan Watt [:jwatt]]

Our current mochitests for directory picking fail if we switch the API to [SecureContext] since mochitest isn't capable of creating a secure context - see bug 1274315.