disabling FORTIFY_SOURCE
Hi, It seems to me that disabling FORTIFY_SOURCE is a mistake. It offers a great many protections, and virtually every distribution has very intentionally turned on this compiler flag by default. Given Wine's size[1], I would argue the benefits[2] outweigh the hassle of rearranging the structures and accessors to not trick the compiler into allocating memory beyond the end of the structure for incoming strings. It has found, at least in other projects, a lot of potential problems, and better yet, has repeatedly turned exploitable vulnerabilities into simple denial of services. I realize it's a bit of a pain to work with given how you're building some of the structures, but I'd like to ask that it not be globally disabled. Thanks, -Kees [1] $ find . -type f -name '*.[ch]' | xargs wc -l | grep total 2678911 total [2] Some details at https://wiki.ubuntu.com/CompilerFlags#-D_FORTIFY_SOURCE=2 but at least the following... Compile time: - static buffer length checks - missed return values - open() checks for missing mode when used with O_CREAT Run time: - dynamic buffer length checks - dynamic format string safety check - dynamic format position safety checks Functions with buffer length (or other) checks: asprintf confstr dprintf fgets fgetws fprintf fread fwprintf getcwd getdomainname getgroups gethostname getlogin_r gets getwd longjmp mbsnrtowcs mbsrtowcs mbstowcs memcpy memmove mempcpy memset pread64 pread printf ptsname_r read readlinkat readlink realpath recv recvfrom snprintf sprintf stpcpy stpncpy strcat strcpy strncat strncpy swprintf syslog ttyname_r vasprintf vdprintf vfprintf vfwprintf vprintf vsnprintf vsprintf vswprintf vsyslog vwprintf wcpcpy wcpncpy wcrtomb wcscat wcscpy wcsncat wcsncpy wcsnrtombs wcsrtombs wcstombs wctomb wmemcpy wmemmove wmempcpy wmemset wprintf -- Kees Cook Ubuntu Security Team
question about standalone tests
Hi! My crypt32 protectdata tests were just added to CVS (thanks!) but they were added without the #ifndef STANDALONE stuff that exists in the recommended example from the lzexpand test code. The steps here: http://www.winehq.com/site/docs/wine-devel/testing-windows don't have a method for doing the build with the free-of-charge windows CLI compiler (which the STANDALONE stuff works fine with). Is there some new way to build standalone tests, or should I send a patch for re-including the #ifndef STANDALONE stuff for the tests? -- Kees Cook@outflux.net
Re: crypt32 [4/4]: (un)protectdata test suite
On Fri, May 20, 2005 at 09:24:56PM +0200, Alexandre Julliard wrote: err:crypt:CryptProtectData CryptAcquireContextW failed Hm. This may be related to not have an initial crypt session for the user. I think I built one (on accident?) while playing with various test scripts, so I suspect something is in the registry. I will investigate. -- Kees Cook@outflux.net
Re: [crypt32] CryptProtectData/CryptUnprotectData helper functions
On Wed, May 18, 2005 at 07:31:03PM +0200, Alexandre Julliard wrote:
+wine_dbg_printf(%s\n,report);
You should use wine_dbg_sprintf here and return a string.
Okay.
+static
+void serialize_dword(DWORD value,BYTE ** ptr)
+{
+/*TRACE(called\n);*/
+
+*((DWORD*)*ptr)=value;
+*ptr+=sizeof(DWORD);
This (and other similar things later on) isn't safe for CPUs that
don't allow unaligned accesses, you have to use memcpy instead.
Ah, whoops. I will fix these.
+pInfo-info0.pbData=strdup(crypt_magic_str);
You don't want to use strdup, you should use HeapAlloc and friends
(especially since you use HeapFree later on).
Is there a HeapAlloc-using version of strdup? I see a while back
someone implemented their own in their own code; is there a common batch
of wine functions, or should I do the same thing?
http://www.winehq.org/hypermail/wine-patches/2004/08/0558.html
StrDup is defined for Windows in .NET and VB, but not for VC, it seems?
+crypt_report_func_input(DATA_BLOB* pDataIn,
+DATA_BLOB* pOptionalEntropy,
+CRYPTPROTECT_PROMPTSTRUCT* pPromptStruct,
+DWORD dwFlags)
+{
+wine_dbg_printf(\tpPromptStruct: 0x%x\n,(unsigned int)pPromptStruct);
+static void
+announce_bad_opaque_data()
+{
+wine_dbg_printf(CryptUnprotectData received the following pDataIn
DATA_BLOB that seems to\n);
+wine_dbg_printf(have NOT been generated by Wine:\n);
+}
You should use the TRACE/FIXME macros here, not raw wine_dbg_printf.
FIXME is sane for the announce_bad_opaque_data, but I'd still like to
use something that doesn't prefix the hexdumps with the function name
for easier readability in the crypt_report_func_input, since there is
already a TRACE/FIXME call being made prior to it's call. Is this okay?
--
Kees Cook@outflux.net
Re: [crypt32] CryptProtectData/CryptUnprotectData helper functions
On Wed, May 18, 2005 at 08:44:17PM +0100, Mike Hearn wrote: It's usually OK to use MESSAGE for that, but if it's a message users might be seeing often it's best to keep it as a WARN or TRACE. Ah-ha, yeah. I should use MESSAGE for that. It's only going to appear if peopl have already turned on trace/warn, or immediately following a FIXME that includes a path. I just want to use it for the readability of the structure. -- Kees Cook@outflux.net
Re: [crypt32] CryptProtectData/CryptUnprotectData helper functions
On Wed, May 18, 2005 at 12:29:58PM -0700, Kees Cook wrote: Take two. Ignore... this patch is broken, and I need to do an update of wine_dbg_printf() - MESSAGE() -- Kees Cook@outflux.net
Re: [crypt32] implementation of CryptProtectData/CryptUnprotectData
On Tue, May 17, 2005 at 02:34:53PM +0900, Mike McCormack wrote: ChangeLog: Implements a best-guess of CryptProtectData/CryptUnprotectData. Wow. That's alot of work :) Heh, yeah, it has been. But, I want it to get accepted, so I'll keep working on it. :) This patch looks OK to me at a quick glance, but if it's not applied, please consider breaking it up. At least that way, you'll have less to submit next time round. Okay, cool. Alexandre, please let me know if breaking it up is prefered, or if you want stuff arranged in some other way. I'm all ears for suggestions. :) -- Kees Cook@outflux.net
Re: crypt32: CryptProtectData/CryptUnprotectData
On Wed, May 04, 2005 at 10:38:40AM +0200, Michael Jung wrote: DATA_BLOB somewhere. He also has to be able to restore in some way the entropy and the description, if he wants to decrypt the DATA_BLOB at some later time. Actually, the description is returned by CryptUnprotectData (and is stored in the clear in the opaque blob). DATA_BLOB's are opaque by nature. Applications should not expect anything of the format. So there is no benefit in trying to mimic the Windows data format. (Sometimes MSDN states that a format should be considered opaque, Well, no, applications don't care, but Wine's implementation (and future implementation) of Crypt*protectData should attempt a guess at it. This is what I've got so far: /* * The data format returned by CryptProtectData seems to be something like: DWORD count0; - how many info0_*[16] blocks follow (was always 1) BYTE info0_0[16];- unknown information (was the same between calls) ... DWORD count1; - how many info1_*[16] blocks follow (was always 1) BYTE info1_0[16];- unknown information (different between calls) ... DWORD null0; - NULL end of records? DWORD str_len;- length of WCHAR string including term WCHAR str[str_len]; - The dataDescription value DWORD unknown0; - unknown value (seems large, but only WORD large, same) DWORD unknown1; - unknown value (seems small, less than a BYTE, same) DWORD data_len; - length of data (was 16 bytes) md5? BYTE data[data_len]; - unknown data (fingerprint? changes) DWORD null1; - NULL ? (same) DWORD unknown2; - unknown value (seems large, but only WORD large, same) DWORD unknown3; - unknown value (seems small, less than a BYTE, same) DWORD salt_len; - salt length(?) - was 16 bytes==128b md5? BYTE salt[salt_len]; - salt for symmetric encryption (changes) DWORD cipher_len; - length of cipher(?) data - was close to plain len, symmetric block ciphers like 3DES are in 40 byte chunks, I think BYTE cipher[cipher_len]; - cipher text? (changes) DWORD fingerprint_len; - length of fingerprint(?) data BYTE fingerprint[fingerprint_len]; - SHA1 fingerprint(?) (20 bytes, changes) */ Like already said, we don't have access to a hash of the user's login password, so we can't provide real security here. Therefore I think we should not try to pretend it. IMO you should'nt do any encryption. Just pass back the DATA_BLOB. I think what I'm going to do is use (for now) the uid appended to a string that includes the time. Maybe md5 it for fun. I really don't know yet. (Before encryption, though, I have to add on the entropy.) Something like: sprintf(salt,Wine:%u:%u:,uid,time()); is accessible by the Crypt* family of API's in advapi32.dll. All that is necessary for this to implement should be available in wine already. Cool, that's good news. I haven't had the chance to review that stuff yet. -- Kees Cook@outflux.net
short-circuting a dialog box?
I'm trying to figure out if there is a way to short-circuit a dialog box. Basically, I want to traps calls to DialogBoxParam, pump calls into lpDialogFunc for dialog init, and then clicking of the Ok button, and finally trap calls to EndDialog. It seems that this is ... hard. :) There is a lot of resource loading, window initialization code, etc that goes on inside DialogBoxParam, and I'd need to handle all of that, I think. There even appears to be issues with 16 vs 32 bit handler addresses, etc. Looks really ugly. Is there a simpler way to programatically click Ok on a dialog box? (The dialog box coming from code that I don't have source for...) -- Kees Cook@outflux.net
Re: short-circuting a dialog box?
On Tue, May 03, 2005 at 11:56:35AM +0300, Shachar Shemesh wrote: For simple things, merely sending the dialog a WM_COMMAND with the right parameters will do it for you. You can programatically find the dialog using FindWindow. Ah-ha, yes. I ended up using EnumWindows (filtering out the HWND from GetTopWindow). Thanks! WM_COMMAND, IDOK, 0 did the trick. Now, a final note, is there any way to stop a dialog from displaying itself? (i.e., let dialogs become active, but not draw themselves?) The ttydrv doesn't like trying to open the dialog: fixme:ttydrv:TTYDRV_GetBitmapBits (0x7d, 0x76d3501c, 128): stub -- Kees Cook@outflux.net
Re: short-circuting a dialog box?
On Tue, May 03, 2005 at 07:38:26PM +0200, [EMAIL PROTECTED] wrote: YOu could trap it#s onshow event but be careful since most dlgs are created as modal , you could end up hanging your process. How would I go about capturing that? (Or, how would I hook the event handler?) -- Kees Cook@outflux.net
Re: short-circuting a dialog box?
On Tue, May 03, 2005 at 08:29:10PM +0200, [EMAIL PROTECTED] wrote: It appears you really have very little knowlege of win32 API programming, I'm not sure this is the place for you to start. That's true. I should probably take this off channel. Is this a wine problem ? If so in what way. It's not a Wine problem exactly. More of a make it work with Linux via Wine issue. I've been developing against the Wine libraries (I don't use Windows natively). The tool I've been writing loads and runs DirectShow DLLs via the IFileSourceFilter APIs: http://outflux.net/software/pkgs/srcfilter/ In the process, I've been writing patches to wine to fully implement some of the calls made by the DLLs I've tried. In an effort to make srcfilter as easy to use as possible, I've been trying to make it as non-interactive as possible, hence my desire to make the dialogs never display, not require X11, etc. I need the dialog to think it's running, though. -- Kees Cook@outflux.net
Re: crypt32: CryptProtectData/CryptUnprotectData
On Tue, May 03, 2005 at 01:58:18PM -0700, Juan Lang wrote: Someone previously posted pretty good information about the format of CryptProtectData on MSDN. I think it should be possible to implement a close facsimile, except that the user's credentials (password) would be missing in Wine since there isn't any at the moment. If you have time to do this, it would be ideal. Failing that, doing any stateless transformation (e.g. no change at all, or xor-ing with some magic value) would be preferrable to storing this data in the registry, if I understood Alexandre's comments correctly. Okay. The article (while very good: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnsecure/html/windataprotection-dpapi.asp) doesn't fully address the opaque data blob. I have taken it apart and found some common lengths, etc, so I think I can produce a reasonable guess at it, so that the future implementations will have very little to change. The clues in the section Protected data blob is what I'm using for the guesses at the data blob format sections. I should have a new patch ready soon. Luckily all the tests and documentation will remain valid. :) We're trying to avoid proliferation of OpenSSL in Wine. Relying on CryptoAPI is a safer bet. Is there somewhere I can find details on what's been completed in the CryptoAPI? The http://winehq.com/site/winapi_stats page say it's at 21%. Thanks for all the clarification. I appreciate it! -- Kees Cook@outflux.net
Re: crypt32: CryptProtectData/CryptUnprotectData
On Wed, Apr 13, 2005 at 12:16:44PM +0200, Alexandre Julliard wrote: I don't understand while you come up with such an elaborate scheme of storing things in the registry when it's clearly not the way this thing is supposed to work. If you can't figure out what Windows does, then just xoring the data with 0xdeadbeef or something like this would be at least as secure as your solution, and would actually be much closer to the proper behavior. Mostly I did this because there is some optional data (description, entropy). I didn't want to have to invent a data format to store all of that in, so I used the registry to do it instead. Another reason I did it this way was so that it was easily to examine and change the information getting passed back from the Crypt*Data functions. But I suppose, I can just use FIXME's for this. I don't like the ssh-agent idea because not everyone uses ssh-agent. If inventing a data format and XORing stuff is prefered, I can write it that way. What direction should I take this? -- Kees Cook@outflux.net
Re: crypt32: CryptProtectData/CryptUnprotectData take 2
On Wed, Apr 06, 2005 at 10:29:37AM +0900, Mike McCormack wrote: The best way to write a test is to look at some of the test cases that are there already. Write and run the test under Windows, and make sure it passes on Windows first. The test is something like this: Ah! Whoops, I didn't think to try that originally. I was just about to send my new patch too. [time passes...] /* try with incorrect parameters */ ok(!CryptProtectData(NULL,NULL,NULL,NULL,NULL,0,NULL), crypt protect data should fail\n); ok(GetLastError() == ERROR_INVALID_PARAMETER, error returned incorrect\n); Ah-ha, I need to make calls to SetLastError then. I wasn't doing that either. Okay, 3rd time's the charm. This patch includes full documentation, as well as the test suite which passes both on Wine and Windows. -- Kees Cook@outflux.net
Re: crypt32: CryptProtectData/CryptUnprotectData take 3
On Wed, Apr 06, 2005 at 11:08:12PM +0900, Dmitry Timoshkov wrote: It's better to keep alphabetical order of .spec file entries. Oh, whoops. I just blindly grouped them. Fixed. if you are not going to conditionally include headers using '#ifdef HAVE_xxx' there is no need to include config.h. Good point. This was copied from elsewhere. I've cleaned up the header files. Registry keys should be closed by RegCloseKey, not CloseHandle (here and everywhere else). Whoops. Okay, fixed. If you are going to test last error value after an API call it's a usual practice to set the error first to some invalid value, 0xdeadbeef works fine. Added. Thanks for all the suggestions from everyone! I'm glad you're willing to put up with me. :) Take 4 on it's way. -- Kees Cook@outflux.net
Re: Remove reference to nonexistant strmif.h
On Mon, Apr 04, 2005 at 10:24:53PM -0400, Dimitrie O. Paun wrote: ChangeLog Remove reference to nonexistant strmif.h +++ include/Makefile.in 5 Apr 2005 02:23:26 - - strmif.h \ /src/wine-cvs/include$ ls strmif.h strmif.h ? it's there for me. :) -- Kees Cook@outflux.net
Re: crypt32: CryptProtectData/CryptUnprotectData take 2
On Tue, Apr 05, 2005 at 02:32:11PM +0900, Mike McCormack wrote: The new patch looks good. I should have mentioned before that writing a test case will help your patch be accepted. Did you have any test code about that you could turn into a test case for your newly implemented functions? Sure, I can write something. I'll look around for docs on how to run tests -- I didn't find that when I looked around this morning. Also: I realize I should provide full documentation for the actual Windows API calls themselves. I documented everything BUT those. :) What's the convention for the number after the API name? I've seen some with numbers, and some with just an @ sign? -- Kees Cook@outflux.net
Re: black-box implementation of CryptProtectData/CryptUnprotectData
On Sun, Apr 03, 2005 at 11:04:53PM -0500, James Hawkins wrote: On Apr 3, 2005 10:12 PM, Kees Cook [EMAIL PROTECTED] wrote: To store the triplets, these functions use the registry: Registry Layout: HKEY_CURRENT_USER\Software\Wine\Crypt\ProtectData\Map\[index] Cipher: HEX string Entropy: HEX string DataDescription: WCHAR Plain: HEX string I'll have to be honest that I haven't looked over the code yet, but I do have a concern. Do these values have to be stored under Software\Wine? That location is for wine-specific configuration options and not api use, unless I'm mistaken. Do we know if this data is stored in the registry in windows, and if so where is it stored? (because I know it's not Software\Wine ;-) Windows doesn't store the results anywhere: it's just a symmetric crypto function. Since we don't know the function, we have to store the original data somewhere so we can return it later. Since this is entirely a Wine-only implementation of the encryption, I wanted to put it somewhere in the registry totally separate from all the other keys. Within the Wine tree seemed like the best place. -- Kees Cook@outflux.net
Re: black-box implementation of CryptProtectData/CryptUnprotectData
On Sun, Apr 03, 2005 at 11:56:34PM -0500, James Hawkins wrote: we do it should be encapsulated. What I'm getting at is that just because this implementation is wine-specific doesn't mean some of the implementation data should go in Software\Wine. Software\Wine is where the configurations of the wine program itself, not its implementation, are contained. See what I mean? btw it's great that Sure, that's fine by me. I wasn't really sure where to put it, but it's easy to change; it's just a path at the top of the file. In looking around at other examples, it seemed the most sensible. Is there a normal implementation-data-storage area for Wine? I just wanted to avoid clashing with any other registry items. I'm open to any suggestions. :) you wrote this code and submitted it. Sending the code in is the big first step. I just think we should get a community opinion on this matter (ya never know, everyone may disagree with me!). Keep up the good coding. Thanks! -- Kees Cook@outflux.net
Re: black-box implementation of CryptProtectData/CryptUnprotectData
On Mon, Apr 04, 2005 at 03:01:53PM +0900, Mike McCormack wrote: Perhaps you could make it work right by using a key stored in ssh-agent? Well, by working right, it means that taking a cipher/entropy from Windows and calling CryptUnprotectData on it in Wine would return the plain text. This isn't going to be possible until we know what Windows keys off of to tie it to a machine and user. I figure the first step is to make the functions work within Wine, then if the encryption is ever understood, the calls can be replaced. You don't need to try patch ChangeLog, because it's going to change alot as patches are applied. Just write the message you want to put in there. Okay, cool. +hr = HRESULT_FROM_WIN32(RegOpenKeyExW(HKEY_CURRENT_USER, wszProtectDataMap, 0, KEY_READ, hkeyMap)); +if (!SUCCEEDED(hr)) Why do you convert the error code to a HRESULT here? Since you don't do it elsewhere in your code, why not compare the returned value to ERROR_SUCCESS, like you do below? Well, mostly I was copying from other examples I found, especially the filtergraph code in dlls/quartz. I'm happy to change that, of course. :) Personally, I prefer the following, as it makes the lines shorter, makes it easier to add a printf(%ld\n,r); and makes the comparison more obvious. r = RegEnumKeyExW(hkeyMap, ... if( r != ERROR_SUCCES ) break; Okay, I can clean this up. You don't need the WINE_TRACE_ON() check, because TRACE already does that for the default debug channel, so the following is the same: Actually, I did that to avoid the line prefix that TRACE adds. All the stuff where I call the dbg functions directly are part of helper functions, and seeing their names is confusing while watching a Protect/Unprotect session. Sometimes you used KR style brackets and indenting, sometimes you used ANSI C style. It's better to choose one or the other and stick to it. Sorry about that. I tried to stick to what seemed to be the wine style, with the braces on separate lines. However, that's not what I'm used to, so a few of mine snuck in. :) +return SUCCEEDED(RegSetValueExW(hkeyOpen,wszName,0,dwType, +pData.pbData,pData.cbData)); The SUCCEEDED() macro is only for HRESULT values, so the above is going to succeed in alot of cases where it shouldn't. Doesn't RegSetValueExW return an HRESULT? In an effort to maintain portability, we don't use C99 style variable declarations. Ah, dang. I tried to clean those up too when I was reading the Patch how-to. I'll clean all this up, thanks very much! BTW: what is your opinion on where to store the triplets in the Registry? -- Kees Cook@outflux.net
Re: black-box implementation of CryptProtectData/CryptUnprotectData
On Tue, Apr 05, 2005 at 01:07:14AM +0900, Mike McCormack wrote: It's probably better to keep it consistent with what the rest of Wine does. I'd really like to push back on this. The traces become unreadable as the various function names change. I think the debugging as I have it is more useful than how it looks with only TRACE calls. The top-level function spits out a TRACE to identify the caller, and then all the helper functions report the data structures. It seems like you need to investigate what it does on Windows and the MSDN description of the function a bit more. The description on MSDN indicated that they used a per user key generated when the user logs in. I already have, and decided it was best to avoid a more detailed investigation for fear of DMCA joy. They key against at least user, machine, and time, since multiple calls with the same plain/entropy produces different ciphers. My implementation intentionally avoids any encryption at all. :) I like to think of it as a good first step to getting the real functions. With what I've got, a program can run normally. I'll be sending version 2 of my patch in a little while. It's got your suggestions incorporated, and a small bug fix. -- Kees Cook@outflux.net
