Bug 158909 – Add FileSystem API feature to determine device ids file paths.

• Status: NEW
• Component: DOM

[Pranjal Jumde]

Add FileSystem API feature to determine device ids of filesystems on which file:// urls are stored

[Pranjal Jumde]

Created attachment 281630 [details]
Patch

[Pranjal Jumde]

Created attachment 281631 [details]
Patch

[WebKit Commit Bot]

Attachment 281631 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/win/FileSystemWin.cpp:141:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 281630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281630&action=review

> Source/WebCore/platform/FileSystem.h:140
> +WEBCORE_EXPORT bool getFileSystemDeviceId(const String&, size_t& result);

I think that I know what you intend to use this for, which makes me skeptical of the approach. Isn't a path based check intrinsically racy?

[Pranjal Jumde]

Created attachment 281632 [details]
Patch

[Pranjal Jumde]

(In reply to comment #4)
> Comment on attachment 281630 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281630&action=review
> 
> > Source/WebCore/platform/FileSystem.h:140
> > +WEBCORE_EXPORT bool getFileSystemDeviceId(const String&, size_t& result);
> 
> I think that I know what you intend to use this for, which makes me
> skeptical of the approach. Isn't a path based check intrinsically racy?

This code eliminates the TOCTOU condition because fstat() is applied to file descriptors, not file names. 

https://www.securecoding.cert.org/confluence/display/c/POS35-C.+Avoid+race+conditions+while+checking+for+the+existence+of+a+symbolic+link

[Alexey Proskuryakov]

I do not think that this is correct. The race is not between lines of code in this new function, but between when the check is made and when the file is re-opened by CFNetwork for reading.

In other words, file system identifier should be retrieved by CFNetwork, not by WebKit.

[Pranjal Jumde]

Created attachment 281635 [details]
Patch

[Pranjal Jumde]

Created attachment 281639 [details]
Patch

[Pranjal Jumde]

Created attachment 281641 [details]
Patch

[Brent Fulgham]

(In reply to comment #7)
> I do not think that this is correct. The race is not between lines of code
> in this new function, but between when the check is made and when the file
> is re-opened by CFNetwork for reading.
> 
> In other words, file system identifier should be retrieved by CFNetwork, not
> by WebKit.

I wonder if we could capture file system identifier at the start of load, and require that CFNetwork (or whatever network stack we are using) validates that the identifier is unchanged at load time?

[Pranjal Jumde]

(In reply to comment #11)
> (In reply to comment #7)
> > I do not think that this is correct. The race is not between lines of code
> > in this new function, but between when the check is made and when the file
> > is re-opened by CFNetwork for reading.
> > 
> > In other words, file system identifier should be retrieved by CFNetwork, not
> > by WebKit.
> 
> I wonder if we could capture file system identifier at the start of load,
> and require that CFNetwork (or whatever network stack we are using)
> validates that the identifier is unchanged at load time?

Even if you have the file handles open it doesn’t matter because CFNetwork uses the urls to fetch data. 

I did not know that even local file loads go through CFNetwork, is there a reason why this is done?  

Making a change to the CFNetwork API for such a specific case does not make sense. There is an easier way to fix this. Once the WebContent receives the data in WebResourceLoader::didRecieveData, for local URLs we verify the content and the device ids and continue if the data we received is not changed.

Thoughts?

[Alexey Proskuryakov]

> I did not know that even local file loads go through CFNetwork, is there a reason why this is done?  

One thing this does is synthesize media type. I don't know if there are other or better reasons.

> There is an easier way to fix this.

I do not understand this proposal. Surely it can be swapped back with good timing?

[Pranjal Jumde]

(In reply to comment #13)
> > I did not know that even local file loads go through CFNetwork, is there a reason why this is done?  
> 
> One thing this does is synthesize media type. I don't know if there are
> other or better reasons.
> 
> > There is an easier way to fix this.
> 
> I do not understand this proposal. Surely it can be swapped back with good
> timing?

Since we are verifying the data returned by CFNetwork with the contents of the file while retrieving the device id we will figure out if anything changed or was swapped.

[Alexey Proskuryakov]

Are you saying that we will read the whole file again using a different code path? That doesn't sound like a good solution.

[Brady Eidson]

Comment on attachment 281641 [details]
Patch

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.