Adding wrappers to Cryptui/Crypt32 and structures to WinCrypt. #915

nahzor · 2018-01-31T19:21:48Z

No description provided.

nahzor · 2018-01-31T19:35:10Z

Working on fixing these. I had run 'ant dist test' but not 'test-platform'.

matthiasblaesing

Thank you for your work Roshan! I added comments to the bindings, as some of these need work to be correct.

Please have a look at the comments and see if they are helpful for you. If you find something unclear, feel free to ask.

matthiasblaesing · 2018-02-01T18:57:04Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public int cMsgCert;
+ public PCERT_CONTEXT rgpMsgCert;
+ public int cMsgCrl;
+ public CRL_CONTEXT rgpMsgCrl;


This looks strange - accoring to the documentation:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa381468(v=vs.85).aspx
this is a Pointer, not the structure itself.

matthiasblaesing · 2018-02-04T11:57:07Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public static class CTL_ENTRY extends Structure {
+ public DATA_BLOB SubjectIdentifier;
+ public int cAttribute;
+ public CRYPT_ATTRIBUTE rgAttribute;


According to the documentation this is an array of CRYPT_ATTRIBUTE. This binding would not even work with a single CRYPT_ATTRIBUTE, as the attribute is not embedded in the structure. This might help:

https://www.eshayne.com/jnaex/index.html?example=16

matthiasblaesing · 2018-02-04T11:59:55Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public int cbSize;
+ public CRL_CONTEXT pBaseCRLContext;
+ public CRL_CONTEXT pDeltaCRLContext;
+ public CRL_ENTRY pCrlEntry;


All of the above elements are defined as pointers to structures and not embedded. These should be defined as CRL_CONTEXT.ByReference and CRL_ENTRY.ByReference

matthiasblaesing · 2018-02-04T12:06:12Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public CRL_CONTEXT pBaseCRLContext;
+ public CRL_CONTEXT pDeltaCRLContext;
+ public CRL_ENTRY pCrlEntry;
+ public boolean fDeltaCrlEntry;


This is a BOOL on the native side. To make this work you'll need to specify a type mapper to get this correctly mapped. See SSPI.SecPkgInfo. There the W32APITypeMapper.DEFAULT is used, that mapper not only maps java boolean to native BOOL, it also maps string correctly to a WString if the unicode binding is used.

This also applies to other structures below.

matthiasblaesing · 2018-02-04T12:10:50Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public int cbSize;
+ public int dwRevocationResult;
+ public LPSTR pszRevocationOid;
+ public LPVOID pvOidSpecificInfo;


If the info should be supplied from the java side I would think about using a plain Pointer, that way you can prepare a Memory object and set it here.

This is not used anymore but i've updated the type.

matthiasblaesing · 2018-02-04T13:06:03Z

contrib/platform/src/com/sun/jna/platform/win32/Crypt32.java

+ */
+ boolean CertGetCertificateChain(Pointer hChainEngine, CERT_CONTEXT pCertContext, Pointer pTime,
+ Pointer hAdditionalStore, CERT_CHAIN_PARA pChainPara, int dwFlags, Pointer pvReserved,
+ CERT_CHAIN_CONTEXT ppChainContext);


It would be good if a subclass of HANDLE would be introduces for hChainEngine and that the two predefined engines HCCE_CURRENT_USER and HCCE_LOCAL_MACHINE are bound.

pTime should be a FILETIME.

I would consider introducing HCERTSTORE as a subclass for HANDLE for the hAdditionalStore parameter.

It would be good if a unittests would be added, that verifies, that the output parameter ppChainContext is correctly filled.

Updated these.

matthiasblaesing · 2018-02-04T13:07:30Z

contrib/platform/src/com/sun/jna/platform/win32/Crypt32.java

+ * @return If the function succeeds, the return value is TRUE. If the function
+ * fails, the return value is FALSE.
+ */
+ boolean CertCloseStore(HANDLE hCertStore, int dwFlags);


See comment for CertGetCertificateChain the hCertStore parameter is used inconsistently. It would be good if this would be unified.

matthiasblaesing · 2018-02-04T13:18:46Z

contrib/platform/src/com/sun/jna/platform/win32/Crypt32.java

+ * null character. If psz is NULL or csz is zero, returns the required
+ * size of the destination string.
+ */
+ int CertNameToStr(int dwCertEncodingType, Pointer pName, int dwStrType, char[] psz, int csz);


pName is a PCERT_NAME_BLOB which is typedeffed from DATA_BLOB. Can the correct type be used here?

The char[] mapping for psz is problematic. This is correct if the system property w32.ascii is not set. This property determines if the Ascii or the unicode variant of the type- and functionmapper is used by default.

In this case I'd advise this: Bind CertNameToStr with Pointer as psz parameter and add utility method to Crypt32Util. That function would determine the needed size of the memory block to hold the name (depending on the value of the mentioned system property the factor 2 (unicode in windows is UCS2), allocate that memory, call the function and read the string from the memory.

See the comment about the return value:

Returns the number of characters converted, including the terminating null character.
If psz is NULL or csz is zero, returns the required size of the destination string.

matthiasblaesing · 2018-02-04T13:21:46Z

contrib/platform/src/com/sun/jna/platform/win32/Crypt32.java

+ * the policy.
+ */
+ boolean CertVerifyCertificateChainPolicy(int pszPolicyOID, CERT_CHAIN_CONTEXT pChainContext,
+ CERT_CHAIN_POLICY_PARA pPolicyPara, CERT_CHAIN_POLICY_STATUS pPolicyStatus);


pszPolicyOID is a string, not an int. Beware - this is a LPCSTR so must always be called with ascii type mapping!

Since the methods in Crypt32 have params for both UNICODE and ASCII, would it be okay to keep the type mapper as the DEFAULT one and use 'com.sun.jna.platform.win32.WTypes.LPSTR' for pszPolicyOID?

You can do this, but then you need to fix it first. LPSTR is defined as:

typedef char CHAR; typedef /* [string] */ __RPC_string CHAR *LPSTR;

So it is an array of chars. The current definition in WTypes.java maps it to w_char. This should to it:

public static class LPSTR extends PointerType { public static class ByReference extends LPSTR implements Structure.ByReference { } public LPSTR() { super(Pointer.NULL); } public LPSTR(Pointer pointer) { super(pointer); } public LPSTR(String value) { this(new Memory(value.length() + 1L)); this.setValue(value); } public void setValue(String value) { this.getPointer().setString(0, value); } public String getValue() { Pointer pointer = this.getPointer(); String str = null; if (pointer != null) str = pointer.getString(0); return str; } @Override public String toString() { return this.getValue(); } }

Please test this!

Thanks. I start working on this and push out the changes i made for the other comments.

Updated and tested this.

matthiasblaesing · 2018-02-04T13:24:35Z

contrib/platform/src/com/sun/jna/platform/win32/Cryptui.java

+ * CertFreeCertificateContext function.
+ */
+ CERT_CONTEXT CryptUIDlgSelectCertificateFromStore(HANDLE hCertStore, HWND hwnd, String pwszTitle,
+ String pwszDisplayString, int dwDontUseColumn, int dwFlags, PointerType pvReserved);


The parameter hCertStore should be checked - see above comments. For the string mappings beware! these are defined as LPWSTR and the binding to String is only correct if w32.ascii is not set/set to false or the whole binding is mapped with the W32APITypeMapper.UNICODE.

matthiasblaesing · 2018-02-05T18:15:46Z

I missed another thing: This is a big enhancement, you should add an entry to CHANGES.md. Please have a look at the other entries for the general structure.

nahzor · 2018-02-06T13:55:33Z

Thank you for the comments, Matthias. I will start working on these.

nahzor · 2018-02-08T23:01:38Z

Hi Matthias, Could you take a look at the changes again and let me know if i need to makes any more changes?

J6ey · 2018-02-10T02:26:32Z

contrib/platform/src/com/sun/jna/platform/win32/WTypes.java

@@ -179,7 +179,7 @@ public String getString() {
 }

 public static class LPSTR extends PointerType {
- public static class ByReference extends BSTR implements


Are you sure you don't extend BSTR?

This is correct. The inner class LPSTR.ByReference is a LPSTR class marked as ByReference (the latter is a marker interface, that forces JNA to pass the value as a reference).

matthiasblaesing

I went through the code again. The main thing I noticed was the mapping of array elements. If the structures are read-only (info only flows from native to java), you can just add a getter to get the whole array via

Structure#toArray

If the info is also set from the Java side, then it is more a question when synchronization happens. Structures are automaticly synchronizing the java mirrors, when a transtion Java<->Native or Native<->Java happens. For arrays this is difficult....

matthiasblaesing · 2018-02-10T10:36:04Z

contrib/platform/src/com/sun/jna/platform/win32/Crypt32.java

+ * function fails, the return value is zero (FALSE).
+ */
+ boolean CryptSignMessage(CRYPT_SIGN_MESSAGE_PARA pSignPara, boolean fDetachedSignature, int cToBeSigned,
+ Pointer rgpbToBeSigned, Pointer rgcbToBeSigned, Pointer pbSignedBlob, IntByReference pcbSignedBlob);


rgcbToBeSigned is an array of DWORD/int
rgpbToBeSigned is an array of Pointers

I would expect, that the user setups one or multiple Memory objects that hold the data to be signed and that binding would match this expected usage better.

matthiasblaesing · 2018-02-10T10:43:24Z

contrib/platform/src/com/sun/jna/platform/win32/WTypes.java

@@ -179,7 +179,7 @@ public String getString() {
 }

 public static class LPSTR extends PointerType {
- public static class ByReference extends BSTR implements


This is correct. The inner class LPSTR.ByReference is a LPSTR class marked as ByReference (the latter is a marker interface, that forces JNA to pass the value as a reference).

matthiasblaesing · 2018-02-10T10:47:23Z

contrib/platform/test/com/sun/jna/platform/win32/Crypt32Test.java

+ // You can use a command similar to the following to create a
+ // certificate that can be used with this example:
+ //
+ // makecert -n "cn=cryptsigntest" -sk cryptsigntest -ss my


What do you think about creating the certificates on the fly and add them to the user stored. At test shutdown these could be removed again.

That way you could really test the functions.

I am having some issues with implementing the CryptAcquireContext wrapper in advapi32, which is part of the workflow for creating a self signed certificate. I will update this after i figure out the issue.

matthiasblaesing · 2018-02-10T10:50:14Z

contrib/platform/test/com/sun/jna/platform/win32/Crypt32Test.java

+ }
+ IntByReference pcbSignedBlob2 = new IntByReference(0);
+ Crypt32.INSTANCE.CryptSignMessage(sigParams, false, messages2.length, rgpbToBeSigned2, rgcbToBeSigned2, Pointer.NULL, pcbSignedBlob2);
+ int signedSize2 = pcbSignedBlob2.getValue();


I would not use StringArray here - its byte arreay, that are signed, so I would use a Memory (see my comment in the CryptSignMessage method).

See also the comment in testCertGetCertificateChain for usage of a generated certificate.

matthiasblaesing · 2018-02-10T11:33:19Z

contrib/platform/src/com/sun/jna/platform/win32/Crypt32Util.java

+ */
+ public static Pointer allocateTCharMemory(int size) {
+ size = Boolean.getBoolean("w32.ascii") ? size : size * 2;
+ return new Memory(size);


Is this used somewhere? The size looks off by one (if not otherwise stated I expect strings to be terminate by a NULL byte. So size needs to be corrected, as size denotes number of characters, not bytes.

I would not use this function and add wrappers for the functions to Crypt32Util that handle the allocation inline.

I had misunderstood your last comment about CertNameToStr. I've updated it to have a utility wrapper method for it instead of this.

matthiasblaesing · 2018-02-10T12:22:12Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public CRYPT_ALGORITHM_IDENTIFIER HashAlgorithm;
+ public Pointer pvHashAuxInfo;
+ public int cMsgCert;
+ public PCERT_CONTEXT.ByReference rgpMsgCert;


This is an array.

I've been having some issue getting the tests to pass after changing these to pointers and adding setters that handle conversion from arrays. I'll update this in the next commit once i figure out the right way to have it.

I had used this example to create the structure and test: https://www.eshayne.com/jnaex/index.html?example=6

I am having difficulty(Invalid memory access) setting it to a pointer and getting it to work. But this seems to work:
'''

public int cMsgCert; public PCERT_CONTEXT.ByReference rgpMsgCert; public void setRgpMsgCert(CERT_CONTEXT.ByReference[] contexts) { PCERT_CONTEXT.ByReference certContextArray = new PCERT_CONTEXT.ByReference(); PCERT_CONTEXT[] certContextArrayValues = (PCERT_CONTEXT[]) certContextArray.toArray(contexts.length); for(int i = 0; i<contexts.length; i++) { certContextArrayValues[i].certContext = contexts[i]; } this.cMsgCert = contexts.length; this.rgpMsgCert = certContextArray; }

'''
If this implementation sounds good to you, i'll implement the same for the rest of the array members in this structure and test them.
(I think this structure is the last change i have to make. I've pushed out changes for the rest.)

matthiasblaesing · 2018-02-10T12:22:20Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public int cMsgCert;
+ public PCERT_CONTEXT.ByReference rgpMsgCert;
+ public int cMsgCrl;
+ public CRL_CONTEXT.ByReference rgpMsgCrl;


This is an array.

matthiasblaesing · 2018-02-10T12:22:35Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public int cMsgCrl;
+ public CRL_CONTEXT.ByReference rgpMsgCrl;
+ public int cAuthAttr;
+ public CRYPT_ATTRIBUTE.ByReference rgAuthAttr;


This is an array.

matthiasblaesing · 2018-02-10T12:22:43Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public int cAuthAttr;
+ public CRYPT_ATTRIBUTE.ByReference rgAuthAttr;
+ public int cUnauthAttr;
+ public CRYPT_ATTRIBUTE.ByReference rgUnauthAttr;


This is an array.

matthiasblaesing · 2018-02-10T12:23:34Z

contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java

+ public int cUnauthAttr;
+ public CRYPT_ATTRIBUTE.ByReference rgUnauthAttr;
+ public int dwFlags;
+ public int dwInnerContentType;


The MSDN sees two further members here:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa381468(v=vs.85).aspx
HashEncryptionAlgorithm and pvHashEncryptionAuxInfo

nahzor · 2018-02-13T16:55:26Z

Thank you. I'm working on these and will send an update soon.

matthiasblaesing · 2018-02-23T21:45:42Z

Thank you for the updates. I will not have the time for a full review this weekend, but I rechecked the array structures and I think the approach will work for the plain structure arrays, but there are arrays, that are arrays of pointers to structures. These are the ones I found:

CERT_SIMPLE_CHAIN.rgpElement
CERT_CHAIN_CONTEXT.rgpChain
CERT_CHAIN_CONTEXT.rgpLowerQualityChainContext
CERT_INFO.rgExtension
CRL_ENTRY.rgExtension
CRL_INFO.rgCRLEntry
CRL_INFO.rgExtension
CRYPT_KEY_PROV_INFO.rgProvParam
CRYPT_SIGN_MESSAGE_PARA.rgpMsgCert
CRYPT_SIGN_MESSAGE_PARA.rgpMsgCrl
CRYPT_SIGN_MESSAGE_PARA.rgAuthAttr
CRYPT_SIGN_MESSAGE_PARA.rgUnauthAttr

For CTL_USAGE you could see if you manage to bind it as a plain String[] (with an ASCII type mapper). In principle this should work.

nahzor · 2018-02-23T23:34:51Z

I tried this out for one of the array of pointer to structures and tested it with CryptSignMessage. The following approach works:

        public int cMsgCert;
	public PointerByReference rgpMsgCert;
	
	public void setRgpMsgCert(CERT_CONTEXT.ByReference[] contexts) {
		this.cMsgCert = contexts.length;
		this.rgpMsgCert = new PointerByReference(contexts[0].getPointer());
	}

Could you confirm if this looks okay? Thanks!

Edit: I added a test for retrieving from an array of pointer types (CERT_INFO.rgExtension) after a certificate is retrieved from the store and this seems to retrieve the array properly. Was that the issue you were concerned about?

matthiasblaesing · 2018-03-03T17:56:04Z

I had another look at the bindings and tried to implement the samples from MSDN using these. To get to a testable point I updated structures and bindings. From my perspective the most important point: The modified tests actually test the functionality. The sample programms from the MSDN look like a good place to start:

https://msdn.microsoft.com/en-us/library/windows/desktop/aa382372(v=vs.85).aspx

The primary problem of the API is the massive use of Pointer types and arrays. Arrays are problematic if the are pointers to structures, as the java side needs to keep a strong reference to prevent the structure from premature GC. This can be worked around, if the structure is used only one way (see CRYPT_SIGN_MESSAGE_PARA), but is problematic if the structure is written/modified from native and java side.

I squashed your commits and added my modifications on top:

https://github.com/matthiasblaesing/jna/tree/rosh89-CryptoLib

This is not a complete solution, but it shows what I mean above. When I updated the SSPI bindings, I started with separate extensions of the bindings in a separate project. I implemented the basic functions to check if the bindings are viable.

The result can be seen here:

https://github.com/matthiasblaesing/jna-sspi-sample

I repeat my question: What is the intention of the bindings? Most functions could be easier handled by using the bouncy castle libraries or even the JDK core classes. I would focus on the core of what you want and that can't be archived with pure java.

- CryptSignMessage + CryptVerifyMessageSignature functions and structures rebound and updated testcase to test full cycle - In the structures holding arrays, the basic binding hold now only a pointer and adds a helper, that converts the pointer to the array of structures. Allowing improved bindings to use the pointer to add custom bindings. WinCryptUtil.MANAGED_CRYPT_SIGN_MESSAGE_PARA is a sample for this. - Updated a working test for Crypt32#CertNameToStr - Adding working version of Crypt32Util#CertNameToStr - Use custom PKCS12 Store to test CertGetCertificateChain and update bindings of associated structures - Use Structure#createFieldsOrder and hold fieldOrder in static field instead of recreating it on all calls - Move Indent to 4 spaces (as in surrounding code)

matthiasblaesing · 2018-03-13T19:48:16Z

Merged updated changeset via e939e66

Thank you for working through this.

nahzor · 2018-03-13T19:49:44Z

Thanks Matthias.

matthiasblaesing requested changes Feb 4, 2018

View reviewed changes

J6ey reviewed Feb 10, 2018

View reviewed changes

matthiasblaesing reviewed Feb 10, 2018

View reviewed changes

Adding wrappers to Cryptui/Crypt32 and structures to WinCrypt.

7d72ae4

matthiasblaesing and others added 2 commits March 11, 2018 19:04

Updating CryptuiTest license

90ab70d

nahzor force-pushed the CryptoLib branch from 2597590 to 90ab70d Compare March 12, 2018 22:48

matthiasblaesing closed this Mar 13, 2018

Adding wrappers to Cryptui/Crypt32 and structures to WinCrypt. #915

Adding wrappers to Cryptui/Crypt32 and structures to WinCrypt. #915

Conversation

nahzor commented Jan 31, 2018

nahzor commented Jan 31, 2018 • edited Loading

matthiasblaesing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasblaesing commented Feb 5, 2018

nahzor commented Feb 6, 2018

nahzor commented Feb 8, 2018

Choose a reason for hiding this comment

matthiasblaesing Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

matthiasblaesing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasblaesing Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahzor commented Feb 13, 2018

matthiasblaesing commented Feb 23, 2018 • edited Loading

nahzor commented Feb 23, 2018 • edited Loading

matthiasblaesing commented Mar 3, 2018

matthiasblaesing commented Mar 13, 2018

nahzor commented Mar 13, 2018

nahzor commented Jan 31, 2018 •

edited

Loading

matthiasblaesing Feb 10, 2018 •

edited

Loading

matthiasblaesing Feb 10, 2018 •

edited

Loading

matthiasblaesing commented Feb 23, 2018 •

edited

Loading

nahzor commented Feb 23, 2018 •

edited

Loading