Hi Hans,
I know this patch already got committed.
+BOOL WINAPI CryptUIWizImport(DWORD dwFlags, HWND hwndParent, LPCWSTR pwszWizardTitle, + PCCRYPTUI_WIZ_IMPORT_SRC_INFO pImportSrc, HCERTSTORE hDestCertStore) +{ + static const WCHAR Root[] = {'R','o','o','t',0}; (snip) + if (!(cert = CertCreateCertificateContext(encoding, buffer, size))) + { + WARN("unable to create certificate context\n"); + HeapFree(GetProcessHeap(), 0, buffer); + return FALSE; + } + /* FIXME: verify certificate and determine store name dynamically */ + if (!(store = CertOpenStore(CERT_STORE_PROV_SYSTEM_W, 0, 0, CERT_SYSTEM_STORE_CURRENT_USER, Root))) + { + WARN("unable to open certificate store\n"); + CertFreeCertificateContext(cert); + HeapFree(GetProcessHeap(), 0, buffer); + return FALSE; + } + ret = CertAddCertificateContextToStore(store, cert, CERT_STORE_ADD_REPLACE_EXISTING, NULL);
This doesn't look correct. Why are you always using the root store, and ignoring hDestCertStore? Even if you expect hDestCertStore to be NULL (in which case a default store may make sense), using the Root store seems like it won't do what the user wants. The certificate won't be persisted if it's added to the Root store, because the Root store is only read from the local system. When the process (Outlook) exits, the certificate will no longer exist.
It should be possible to add a test that shows which store the certificate should be added to, if nothing else to satisfy my doubt. --Juan
On Monday 20 October 2008 21:48:37 Juan Lang wrote:
- /* FIXME: verify certificate and determine store name dynamically */
- if (!(store = CertOpenStore(CERT_STORE_PROV_SYSTEM_W, 0, 0,
CERT_SYSTEM_STORE_CURRENT_USER, Root)))
- {
WARN("unable to open certificate store\n");CertFreeCertificateContext(cert);HeapFree(GetProcessHeap(), 0, buffer);return FALSE;- }
- ret = CertAddCertificateContextToStore(store, cert,
CERT_STORE_ADD_REPLACE_EXISTING, NULL);
This doesn't look correct. Why are you always using the root store, and ignoring hDestCertStore? Even if you expect hDestCertStore to be NULL (in which case a default store may make sense), using the Root store seems like it won't do what the user wants. The certificate won't be persisted if it's added to the Root store, because the Root store is only read from the local system. When the process (Outlook) exits, the certificate will no longer exist.
It's my limited manual testing with a self-signed root CA certificate that turned this up on Windows. The certificate is still there after Outook is closed.
It's an absolute minimal implementation and you are right that we need to find out what determines the store for the whole range of certificate types. The FIXME comment I put in should really have been a FIXME().
-Hans
It's my limited manual testing with a self-signed root CA certificate that turned this up on Windows. The certificate is still there after Outook is closed.
In Windows? Sure. In Wine? I can't see how that would be the case. (In fact it turns up a crash here for me.) --Juan
On Monday 20 October 2008 22:51:15 Juan Lang wrote:
It's my limited manual testing with a self-signed root CA certificate that turned this up on Windows. The certificate is still there after Outook is closed.
In Windows? Sure. In Wine? I can't see how that would be the case. (In fact it turns up a crash here for me.)
It persists in Windows, yes. Haven't tested Wine, where do you see a crash?
-Hans
It persists in Windows, yes. Haven't tested Wine, where do you see a crash?
In crypt32. I wrote a quick test program that does what your patch does, and it crashes adding the certificate to the root store. I'll send a patch shortly that'll avoid the crash. Nevertheless, this won't do what you want in Wine: the root store is read-only in Wine. --Juan
On Monday 20 October 2008 23:06:35 Juan Lang wrote:
It persists in Windows, yes. Haven't tested Wine, where do you see a crash?
In crypt32. I wrote a quick test program that does what your patch does, and it crashes adding the certificate to the root store. I'll send a patch shortly that'll avoid the crash. Nevertheless, this won't do what you want in Wine: the root store is read-only in Wine.
It may not persist but I could import the certificate fine on Wine. Is there an alternative for the root store? What's involved in making the root store read-write?
-Hans
It may not persist but I could import the certificate fine on Wine. Is there an alternative for the root store? What's involved in making the root store read-write?
You haven't convinced me that Windows does indeed import the certificate to the root store in all cases. Making the root store read-write isn't the right answer. Please write more tests first. --Juan
On Monday 20 October 2008 23:21:44 Juan Lang wrote:
You haven't convinced me that Windows does indeed import the certificate to the root store in all cases. Making the root store
I don't think I said that. I put a fixme in the code that explicitly warns that the store should be determined dynamically.
-Hans
You haven't convinced me that Windows does indeed import the certificate to the root store in all cases. Making the root store
I don't think I said that. I put a fixme in the code that explicitly warns that the store should be determined dynamically.
No, but that's what the code does. What bothers me is that your implementation is correct in only an extremely narrow set of conditions: 1. dwFlags has CRYPTUI_WIZ_NO_UI SET 2. hDestStore is NULL 3. the certificate belongs in the root store The trouble is your patch inserts the certificate in the root store whether or not any of the above conditions is not met. You also ignore the most obvious source of determining the correct destination for the certificate: hDestCertStore.
At least when the source is not a file you fail gracefully. I'd be much happier if you did the same when any of the rest of the conditions is not true, and some tests for the CRYPTUI_WIZ_NO_UI case wouldn't be that hard to write, so why not write them? --Juan
On Tuesday 21 October 2008 16:46:51 Juan Lang wrote:
I don't think I said that. I put a fixme in the code that explicitly warns that the store should be determined dynamically.
No, but that's what the code does. What bothers me is that your implementation is correct in only an extremely narrow set of conditions:
Like I said, it's exactly the set of conditions that happens to satisfy Outlook. The typical scenario is that you can't connect to a secure server because of an invalid certificate and then forcibly import the certificate. The invalid certificates I tried on Windows where added to the root store.
-Hans
Like I said, it's exactly the set of conditions that happens to satisfy Outlook. The typical scenario is that you can't connect to a secure server because of an invalid certificate and then forcibly import the certificate. The invalid certificates I tried on Windows where added to the root store.
But you don't check whether those conditions are true, and you march ahead and install the certificate into the root store whether or not they are true. I'm sorry, but the code is just not correct. Please write some test cases. --Juan
On Tuesday 21 October 2008 19:06:20 Juan Lang wrote:
But you don't check whether those conditions are true, and you march ahead and install the certificate into the root store whether or not they are true. I'm sorry, but the code is just not correct. Please write some test cases.
It's a stub of course, so it doesn't always do the right thing. We have many of these in Wine, and that's OK as long as you are warned about the shortcomings.
If I'm right about typical usage of this function it will do the right thing more often than not, which is pretty good for a stub.
-Hans
On Wed, Oct 22, 2008 at 10:41:00AM +0200, Hans Leidekker wrote:
On Tuesday 21 October 2008 19:06:20 Juan Lang wrote:
But you don't check whether those conditions are true, and you march ahead and install the certificate into the root store whether or not they are true. I'm sorry, but the code is just not correct. Please write some test cases.
It's a stub of course, so it doesn't always do the right thing. We have many of these in Wine, and that's OK as long as you are warned about the shortcomings.
If I'm right about typical usage of this function it will do the right thing more often than not, which is pretty good for a stub.
It is however unsecure when I am reading that right ;)
Ciao, Marcus
On Wednesday 22 October 2008 10:47:25 Marcus Meissner wrote:
It's a stub of course, so it doesn't always do the right thing. We have many of these in Wine, and that's OK as long as you are warned about the shortcomings.
If I'm right about typical usage of this function it will do the right thing more often than not, which is pretty good for a stub.
It is however unsecure when I am reading that right ;)
Yes, that's one of the shortcomings.
-Hans
If I'm right about typical usage of this function it will do the right thing more often than not, which is pretty good for a stub.
I don't think that's typical usage at all: typical usage presents a UI. It's called from elsewhere in cryptui, so it's under the control of the user how frequently this is used. You add a cert to the root store even when a UI is requested. This is clearly incorrect. --Juan
-
Hans Leidekker -
Hans Leidekker -
Juan Lang -
Marcus Meissner