Bug 36472 – Add support to create UUID string.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jian Li]

We need to add support to create UUID string. This is needed to create UUID-based URN for File.urn and other scenarios.

[Jian Li]

Created attachment 51374 [details]
Proposed Patch

[WebKit Review Bot]

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.

[Darin Adler]

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.

[Jian Li]

(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.

[Jian Li]

Created attachment 51768 [details]
Proposed Patch

Introduced separate files UUID.* and placed them in JavaScriptCore.

[Darin Adler]

Comment on attachment 51768 [details]
Proposed Patch

What inspired you to choose JavaScriptCore/wtf rather than WebCore/platform for this?

[Jian Li]

(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.

[Jian Li]

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.

[WebKit Review Bot]

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.

[Jian Li]

Created attachment 52189 [details]
Fix style error

[Dmitry Titov]

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?

[Jian Li]

Created attachment 52200 [details]
Proposed Patch

[Jian Li]

> > 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.

[Dmitry Titov]

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.

[Jian Li]

Committed as http://trac.webkit.org/changeset/56869.

[Jian Li]

(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.

[Adam Barth]

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.

[Adam Barth]

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.

[Jian Li]

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.

[Adam Barth]

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.

[Jian Li]

(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.

[Dmitry Titov]

(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.

[Adam Barth]

> 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.

[Ismail Donmez]

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

[Jian Li]

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

[Jian Li]

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.

[Jian Li]

Created attachment 52787 [details]
Patch to add comment and assert for version 4 UUID

[Adam Barth]

Comment on attachment 52787 [details]
Patch to add comment and assert for version 4 UUID

Excellent.  Thanks for digging into this issue.

[Dmitry Titov]

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/

[Jian Li]

Committed as http://trac.webkit.org/changeset/57239.

[Ismail Donmez]

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

[Eric Seidel (no email)]

This looks to have broken the Qt build.

[Csaba Osztrogonác]

(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.)

[Ismail Donmez]

This patch broke the Qt/WinCE build, the toplevel if should be like

#if OS(WINDOWS) && !OS(WINCE)

[Csaba Osztrogonác]

(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

[Ismail Donmez]

(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!

[Csaba Osztrogonác]

(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.)

[Ismail Donmez]

(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.

[Csaba Osztrogonác]

(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?

[Ismail Donmez]

(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.

[Adam Barth]

> 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.

[Csaba Osztrogonác]

(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.

[Ismail Donmez]

(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.