Summary: | Add support to create UUID string. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jian Li <jianli> | ||||||||||||||
Component: | WebCore JavaScript | Assignee: | Jian Li <jianli> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, dimich, eric, ismail, levin, ossy, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | 36929, 38581 | ||||||||||||||||
Bug Blocks: | 36678 | ||||||||||||||||
Attachments: |
|
Description
Jian Li
2010-03-22 18:04:39 PDT
Created attachment 51374 [details]
Proposed Patch
Attachment 51374 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/text/String.cpp:965: Place brace on its own line for function definitions. [whitespace/braces] [4]
WebCore/platform/text/String.cpp:975: Use 0 instead of NULL. [readability/null] [5]
WebCore/platform/text/String.cpp:976: Use 0 instead of NULL. [readability/null] [5]
WebCore/platform/text/String.cpp:984: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 4 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 51374 [details]
Proposed Patch
This does not belong in the String class's header file. You should give it its own header file unless you can find some other functions it belongs with.
The code to make the strings seems OK. but the opening function brace should be on its own line. Also, we normally put separate functions per platform into separate source files instead of adding them all to one file.
(In reply to comment #3) > (From update of attachment 51374 [details]) > This does not belong in the String class's header file. You should give it its > own header file unless you can find some other functions it belongs with. > > The code to make the strings seems OK. but the opening function brace should be > on its own line. Also, we normally put separate functions per platform into > separate source files instead of adding them all to one file. I will put this in a separate header file, say UUID.h. Since the implementation is more OS-specific, instead of platform-specific, how could we put separate implementations per OS? If this is not likely, I will go with per-platform. Created attachment 51768 [details]
Proposed Patch
Introduced separate files UUID.* and placed them in JavaScriptCore.
Comment on attachment 51768 [details]
Proposed Patch
What inspired you to choose JavaScriptCore/wtf rather than WebCore/platform for this?
(In reply to comment #6) > (From update of attachment 51768 [details]) > What inspired you to choose JavaScriptCore/wtf rather than WebCore/platform for > this? I think it provides basic support to create a UUID string that can be used for different platforms. In addition, the implementation is more OS dependent. How do you think? Correct me if I get what could be put under wtf wrong. Thanks. Created attachment 52115 [details]
Revised Patch
Moved UUID.* from JavaScriptCore/wtf to Webore/platform.
Since the implementation is very simple and OS-specific, I'd rather keep all the implementation in one cpp file.
Attachment 52115 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
File not a recognized type to check. Skipping: "WebCore/WebCore.pro"
WebCore/platform/UUID.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4]
File not a recognized type to check. Skipping: "WebCore/WebCore.vcproj/WebCore.vcproj"
File not a recognized type to check. Skipping: "WebCore/GNUmakefile.am"
File not a recognized type to check. Skipping: "WebCore/WebCore.gypi"
File not a recognized type to check. Skipping: "WebCore/WebCore.xcodeproj/project.pbxproj"
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 52189 [details]
Fix style error
Comment on attachment 52189 [details] Fix style error I think you left the implementation in a single file because it's just a few lines for each, and it's unclear which platform subdir of the WebCore/platform would be the right place for split files... It looks ok in this case, there are precedents (CurrentTime.cpp), so if others won't object, I think it's fine here. Few notes: > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > + Need a short description and bug URL (OOPS!) oops. > diff --git a/WebCore/platform/UUID.cpp b/WebCore/platform/UUID.cpp > +String createCanonicalUUIDString() > +{ > +#if OS(WINDOWS) > + GUID uuid = { 0 }; > + HRESULT hr = CoCreateGuid(&uuid); > + ASSERT(SUCCEEDED(hr)); > + if (FAILED(hr)) > + return String(); Either ASSERT or if (or both) can be removed here. It's either ASSERT or something that can actually happen and we have a code path for that. > +#elif OS(LINUX) > + FILE* fptr = fopen("/proc/sys/kernel/random/uuid", "r"); > + ASSERT(fptr); > + if (!fptr) > + return String(); Same here. > + char uuidStr[37] = {0}; > + fgets(uuidStr, sizeof(uuidStr) - 1, fptr); > + fclose(fptr); > + return String(uuidStr); Here there is no lower(), while in other cases there is. BTW, why do we need to lowercase it? It's not like we are going to produce 2 GUID strings and then try to compare them, right? Created attachment 52200 [details]
Proposed Patch
> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > > + Need a short description and bug URL (OOPS!) > > oops. Removed. > > > diff --git a/WebCore/platform/UUID.cpp b/WebCore/platform/UUID.cpp > > +String createCanonicalUUIDString() > > +{ > > +#if OS(WINDOWS) > > + GUID uuid = { 0 }; > > + HRESULT hr = CoCreateGuid(&uuid); > > + ASSERT(SUCCEEDED(hr)); > > + if (FAILED(hr)) > > + return String(); > > Either ASSERT or if (or both) can be removed here. It's either ASSERT or > something that can actually happen and we have a code path for that. Removed ASSERT. > > > +#elif OS(LINUX) > > + FILE* fptr = fopen("/proc/sys/kernel/random/uuid", "r"); > > + ASSERT(fptr); > > + if (!fptr) > > + return String(); > > Same here. Removed ASSERT. > > > + char uuidStr[37] = {0}; > > + fgets(uuidStr, sizeof(uuidStr) - 1, fptr); > > + fclose(fptr); > > + return String(uuidStr); > > Here there is no lower(), while in other cases there is. BTW, why do we need to > lowercase it? It's not like we are going to produce 2 GUID strings and then try > to compare them, right? In Linux, what we get is the lower-case UUID string so we do not need to do the conversion. The File.urn requires the UUID to be in canonical format, lower case. So I enforce lower case output for this function. Comment on attachment 52200 [details] Proposed Patch r=me One note, please consider while landing: > +#elif OS(LINUX) > + FILE* fptr = fopen("/proc/sys/kernel/random/uuid", "r"); > + if (!fptr) > + return String(); > + char uuidStr[37] = {0}; > + fgets(uuidStr, sizeof(uuidStr) - 1, fptr); > + fclose(fptr); > + return String(uuidStr); If we do want the lowercase only, it probably makes sense to return .lower() here as well. Unless we have a guarantee that all Linux flavors return lowercase already. Committed as http://trac.webkit.org/changeset/56869. (In reply to comment #14) > (From update of attachment 52200 [details]) > r=me > > One note, please consider while landing: > > > +#elif OS(LINUX) > > + FILE* fptr = fopen("/proc/sys/kernel/random/uuid", "r"); > > + if (!fptr) > > + return String(); > > + char uuidStr[37] = {0}; > > + fgets(uuidStr, sizeof(uuidStr) - 1, fptr); > > + fclose(fptr); > > + return String(uuidStr); > > If we do want the lowercase only, it probably makes sense to return .lower() > here as well. Unless we have a guarantee that all Linux flavors return > lowercase already. This has been fixed and landed. Comment on attachment 52200 [details]
Proposed Patch
I believe this code is wrong and should be rolled out.
+ HRESULT hr = CoCreateGuid(&uuid);
This method incorporates private data from the operating system in the guid. If we expose this to web content (e.g., in a file URN), then we'll leak information like the user's MAC address, which is bad news bears.
It's probably better to use a purely random number.
From http://msdn.microsoft.com/en-us/library/aa379205(v=VS.85).aspx [[ For security reasons, it is often desirable to keep ethernet addresses on networks from becoming available outside a company or organization. The UuidCreate function generates a UUID that cannot be traced to the ethernet address of the computer on which it was generated. It also cannot be associated with other UUIDs created on the same computer. If you do not need this level of security, your application can use the UuidCreateSequential function, which behaves exactly as the UuidCreate function does on all other versions of the operating system. ]] I'm not sure what threat model they're considering, but the MAC is used in the computation. This level of uniqueness isn't required here and is dangerous. Per MSDN, the CoCreateGuid function calls the RPC function UuidCreate. And for security reasons, UuidCreate was modified so that it no longer uses a machine's MAC address to generate UUIDs. So I think we're fine here since we do not expose MAC address in the UUID string we create on Windows. What's the advantage of calling these APIs versus using a random number? These APIs can leak a lot of information if not done carefully. (In reply to comment #20) > What's the advantage of calling these APIs versus using a random number? These > APIs can leak a lot of information if not done carefully. The UUID is needed in implementing File.urn as defined in File API spec: http://www.w3.org/TR/FileAPI/#dfn-urn. If there is a security concern on this, we need to talk with the spec writer and people in the working group to get it tuned. (In reply to comment #20) > What's the advantage of calling these APIs versus using a random number? These > APIs can leak a lot of information if not done carefully. Interesting. Do you suggest using random number generator to form a string that looks like UUID? (because File.urn asks for it). I suspect not every random number generator is ok to use here, but I'm not sure I know what kind of information leaks we are trying to prevent. Also, the sequential calls to generate UUID to OS-provided functions practically guarantee to get a different numbers each time, for random numbers we probably need some code to make sure we don't repeat. Reopening the bug until this discussion has a resolution. > Interesting. Do you suggest using random number generator to form a string that > looks like UUID? (because File.urn asks for it). Yes. > I suspect not every random number generator is ok to use here, but I'm not sure > I know what kind of information leaks we are trying to prevent. We've had trouble with even our random number generating leaking too much information. We want to use the platform's cryptographic random number generator here. > Also, the sequential calls to generate UUID to OS-provided functions > practically guarantee to get a different numbers each time, for random numbers > we probably need some code to make sure we don't repeat. If we use enough bits, that won't be an issue. It will be more likely that the atoms in your body spontaneously explode. This broke Qt/WinCE compilation: UUID.obj : error LNK2019: unresolved external symbol StringFromGUID2 referenced in function "class WebCore::String __cdecl WebCore::createCanonicalUUIDString(void)" (?createCanonicalUUIDString@WebCore@@YA?AVString@1@XZ) UUID.obj : error LNK2019: unresolved external symbol CoCreateGuid referenced in function "class WebCore::String __cdecl WebCore::createCanonicalUUIDString(void)" (?createCanonicalUUIDString@WebCore@@YA?AVString@1@XZ) ..\lib\QtWebKit4.dll : fatal error LNK1120: 2 unresolved externals This has already been fixed by ossi. (In reply to comment #24) > This broke Qt/WinCE compilation: > > > UUID.obj : error LNK2019: unresolved external symbol StringFromGUID2 referenced > in function "class WebCore::String __cdecl > WebCore::createCanonicalUUIDString(void)" > (?createCanonicalUUIDString@WebCore@@YA?AVString@1@XZ) > UUID.obj : error LNK2019: unresolved external symbol CoCreateGuid referenced in > function "class WebCore::String __cdecl > WebCore::createCanonicalUUIDString(void)" > (?createCanonicalUUIDString@WebCore@@YA?AVString@1@XZ) > ..\lib\QtWebKit4.dll : fatal error LNK1120: 2 unresolved externals Per wikipedia (http://en.wikipedia.org/wiki/Universally_Unique_Identifier), there're 5 different versions for generating UUID. The version 4 is to use random number only. Version 4 (random) Version 4 UUIDs use a scheme relying only on random numbers. This algorithm sets the version number as well as two reserved bits. All other bits are set using a random or pseudorandom data source. Version 4 UUIDs have the form xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx with hexadecimal digits for x and one of 8, 9, A, or B for y. e.g. f47ac10b-58cc-4372-a567-0e02b2c3d479. On Windows, version 4 UUIDs are used since Windows 2000 (http://msdn.microsoft.com/en-us/library/aa446557.aspx). On MacOSX, version 4 UUIDs are used since Tiger (http://developer.apple.com/mac/library/technotes/tn/tn1103.html#TNTAG8). On Linux, the kernel offers the procfs pseudo-file /proc/sys/kernel/random/uuid that yields versiob 4 UUIDs (http://hbfs.wordpress.com/2008/09/30/ueid-unique-enough-ids/). I will add the comment to the function and put up an assert to check for version 4. (In reply to comment #18) > From http://msdn.microsoft.com/en-us/library/aa379205(v=VS.85).aspx > > [[ > For security reasons, it is often desirable to keep ethernet addresses on > networks from becoming available outside a company or organization. The > UuidCreate function generates a UUID that cannot be traced to the ethernet > address of the computer on which it was generated. It also cannot be associated > with other UUIDs created on the same computer. If you do not need this level of > security, your application can use the UuidCreateSequential function, which > behaves exactly as the UuidCreate function does on all other versions of the > operating system. > ]] > > I'm not sure what threat model they're considering, but the MAC is used in the > computation. This level of uniqueness isn't required here and is dangerous. Created attachment 52787 [details]
Patch to add comment and assert for version 4 UUID
Comment on attachment 52787 [details]
Patch to add comment and assert for version 4 UUID
Excellent. Thanks for digging into this issue.
Comment on attachment 52787 [details] Patch to add comment and assert for version 4 UUID Couple of nits, could be cleaned up on landing: > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > + Add the comment and assert to illustrate that we're generating version 4 "to illustrate" can be omitted. > diff --git a/WebCore/platform/UUID.h b/WebCore/platform/UUID.h > +// Note: for security reason, we should always generate version 4 UUID that use a scheme relying only on random numbers. > +// This algorithm sets the version number as well as two reserved bits. All other bits are set using a random or pseudorandom > +// data source. Version 4 UUIDs have the form xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx with hexadecimal digits for x and one of 8, > +// 9, A, or B for y. Perhaps it was intended to be "... with hexadecimal digit 4 for x and..." > +// > +// On Windows, version 4 UUIDs are used since Windows 2000 (http://msdn.microsoft.com/en-us/library/aa446557.aspx). > +// On MacOSX, version 4 UUIDs are used since Tiger (http://developer.apple.com/mac/library/technotes/tn/tn1103.html#TNTAG8). > +// On Linux, the kernel offers the procfs pseudo-file /proc/sys/kernel/random/uuid that yields versiob 4 UUIDs (http://hbfs.wordpress.com/2008/09/30/ueid-unique-enough-ids/). s/versiob/version/ Committed as http://trac.webkit.org/changeset/57239. Which revision? I can't see the fix in SVN. (In reply to comment #25) > This has already been fixed by ossi. > > (In reply to comment #24) > > This broke Qt/WinCE compilation: > > > > > > UUID.obj : error LNK2019: unresolved external symbol StringFromGUID2 referenced > > in function "class WebCore::String __cdecl > > WebCore::createCanonicalUUIDString(void)" > > (?createCanonicalUUIDString@WebCore@@YA?AVString@1@XZ) > > UUID.obj : error LNK2019: unresolved external symbol CoCreateGuid referenced in > > function "class WebCore::String __cdecl > > WebCore::createCanonicalUUIDString(void)" > > (?createCanonicalUUIDString@WebCore@@YA?AVString@1@XZ) > > ..\lib\QtWebKit4.dll : fatal error LNK1120: 2 unresolved externals This looks to have broken the Qt build. (In reply to comment #32) > This looks to have broken the Qt build. Thx, but it isn't the culprit. :) (But http://trac.webkit.org/changeset/57240 is it, I'm working on it.) This patch broke the Qt/WinCE build, the toplevel if should be like #if OS(WINDOWS) && !OS(WINCE) (In reply to comment #34) > This patch broke the Qt/WinCE build, the toplevel if should be like > #if OS(WINDOWS) && !OS(WINCE) I fixed it for win32: http://trac.webkit.org/changeset/56922 (bug: https://bugs.webkit.org/show_bug.cgi?id=36929 ) , but not for wince. I think the correct solution is adding Ole32 library instead of disable this feature. Proposed fix uploaded into bug: https://bugs.webkit.org/show_bug.cgi?id=36929 (In reply to comment #35) > (In reply to comment #34) > > This patch broke the Qt/WinCE build, the toplevel if should be like > > #if OS(WINDOWS) && !OS(WINCE) > > I fixed it for win32: http://trac.webkit.org/changeset/56922 > (bug: https://bugs.webkit.org/show_bug.cgi?id=36929 ) , > but not for wince. I think the correct solution is > adding Ole32 library instead of disable this feature. > > Proposed fix uploaded into bug: https://bugs.webkit.org/show_bug.cgi?id=36929 I am not sure about DCOM support in WinCE but I'll take your word for it. Thanks! (In reply to comment #36) > I am not sure about DCOM support in WinCE but I'll take your word for it. > Thanks! CoCreateGuid: http://msdn.microsoft.com/en-us/library/ms863892.aspx StringFromGUID2: http://msdn.microsoft.com/en-us/library/ms891281.aspx Determining Supported COM APIs: http://msdn.microsoft.com/en-us/library/ms885910.aspx StringFromGUID2 is supported by all API (Minimal COM Supported APIs, COM Supported APIs, and DCOM Supported APIs), but CoCreateGuid is supported by DCOM only. Should we support lesser API? (I'm not a WinCE expert, not at all.) (In reply to comment #37) > (In reply to comment #36) > > I am not sure about DCOM support in WinCE but I'll take your word for it. > > Thanks! > > CoCreateGuid: http://msdn.microsoft.com/en-us/library/ms863892.aspx > StringFromGUID2: http://msdn.microsoft.com/en-us/library/ms891281.aspx > > Determining Supported COM APIs: > http://msdn.microsoft.com/en-us/library/ms885910.aspx > > StringFromGUID2 is supported by all API (Minimal COM Supported APIs, COM > Supported APIs, and DCOM Supported APIs), but CoCreateGuid is supported by DCOM > only. Should we support lesser API? (I'm not a WinCE expert, not at all.) CoCreateGuid support seems to be implementation dependent. Using StringFromGUID2 might be a better idea. (In reply to comment #38) > CoCreateGuid support seems to be implementation dependent. Using > StringFromGUID2 might be a better idea. It seems CoCreateGuid is necessary, but we can create GUID from MAC address and timestamp with a new function. :) I don't think DCOM is too big expectation from a WebKit user. Or is it? (In reply to comment #39) > (In reply to comment #38) > > CoCreateGuid support seems to be implementation dependent. Using > > StringFromGUID2 might be a better idea. > It seems CoCreateGuid is necessary, but we can create GUID from MAC address and > timestamp with a new function. :) > > I don't think DCOM is too big expectation from a WebKit user. Or is it? Its a big expectation, Qt has its own implementation called qt_CoCreateGuid just because Windows CE fails to implement this right. > It seems CoCreateGuid is necessary, but we can create GUID from MAC address and
> timestamp with a new function. :)
We specifically do not want to use the user's MAC address!
I continue to think we should just generate this string using a secure random
number generator instead of screwing around with these APIs. They invite too
many security mistakes.
(In reply to comment #40) > Its a big expectation, Qt has its own implementation called qt_CoCreateGuid > just because Windows CE fails to implement this right. OK, I respect it. My proposed patch in the another bug is necessary, it is unrelated to DCOM. I can't find qt_CoCreateGuid, but QUuid class ( http://doc.trolltech.com/4.6/quuid.html ) It would be great an other patch to use it for Qt port. (In reply to comment #42) > (In reply to comment #40) > > Its a big expectation, Qt has its own implementation called qt_CoCreateGuid > > just because Windows CE fails to implement this right. > OK, I respect it. My proposed patch in the another bug is necessary, it is > unrelated to DCOM. > > I can't find qt_CoCreateGuid, but QUuid class ( > http://doc.trolltech.com/4.6/quuid.html ) It would be great an other patch to > use it for Qt port. Yeah Qt port should use its own QUuid class, nice find. qt_CoCreateGuid is in qapplication.cpp not a public symbol. |