Adding security functions and RtlNtStatusToDosError to NtDll #738
Conversation
* EqualSid * InitializeSecurityDescriptor * InitializeAcl * AddAce * AddAccessAllowedAce * AddAccessAllowedAceEx * GetAce Added other ways to manipulate and initialize ACL and ACE in WinNT
* GetSecurityDescriptorControl * SetSecurityDescriptorControl * GetSecurityDescriptorDacl * SetSecurityDescriptorDacl * MakeSelfRelativeSD * MakeAbsoluteSD
* GetSecurityDescriptorOwner * SetSecurityDescriptorOwner * GetSecurityDescriptorGroup * SetSecurityDescriptorGroup
| * information, call GetLastError. | ||
| * If either SID structure is not valid, the return value is undefined. | ||
| */ | ||
| boolean EqualSid(PSID pSid1, PSID pSid2); |
There was a problem hiding this comment.
Is there a difference between pSid1.equals(pSid2) and EqualSid(pSid1, pSid2)? The definition of Structure#equals looks reasonable.
There was a problem hiding this comment.
Looks to me that it compares class types then calls Pointer#equals which compares that the native peer memory address is the same. EqualSid compares the contents under those memory address.
There was a problem hiding this comment.
Sorry - I read outdated documentation. dataEquals does the "right thing" anyways - the function is small simple documentated, so lets keep it.
| * function fails, the return value is zero. For extended error | ||
| * information, call GetLastError. | ||
| */ | ||
| boolean GetSecurityDescriptorControl(SECURITY_DESCRIPTOR pSecurityDescriptor, IntByReference pControl, IntByReference lpdwRevision); |
There was a problem hiding this comment.
pControl is a PSECURITY_DESCRIPTOR_CONTROL is defined as:
typedef WORD SECURITY_DESCRIPTOR_CONTROL, *PSECURITY_DESCRIPTOR_CONTROL;Shouldn't this be ShortByReference?
| * function fails, the return value is zero. For extended error | ||
| * information, call GetLastError. | ||
| */ | ||
| boolean SetSecurityDescriptorControl(SECURITY_DESCRIPTOR pSecurityDescriptor, int ControlBitsOfInterest, int ControlBitsToSet); |
There was a problem hiding this comment.
See comment on 331 - ControlBitsOfInterest and ControlBitsToSet are defined to be word-size.
| * function fails, the return value is zero. For extended error | ||
| * information, call GetLastError. | ||
| */ | ||
| boolean GetSecurityDescriptorOwner(SECURITY_DESCRIPTOR pSecurityDescriptor, PointerByReference pOwner, BOOLByReference lpbOwnerDefaulted); |
There was a problem hiding this comment.
There is a PSIDByReference already in the codebase would that be sensible here for pOwner?
| * function fails, the return value is zero. For extended error | ||
| * information, call GetLastError. | ||
| */ | ||
| boolean GetSecurityDescriptorGroup(SECURITY_DESCRIPTOR pSecurityDescriptor, PointerByReference pGroup, BOOLByReference lpbGroupDefaulted); |
There was a problem hiding this comment.
see comment on GetSecurityDescriptorOwner here for pGroup.
| * function fails, the return value is zero. For extended error | ||
| * information, call GetLastError. | ||
| */ | ||
| boolean GetSecurityDescriptorDacl(SECURITY_DESCRIPTOR pSecurityDescriptor, BOOLByReference bDaclPresent, PointerByReference pDacl, BOOLByReference bDaclDefaulted); |
There was a problem hiding this comment.
You are using ACL in the SetSecurityDescriptorDacl you could check if WinNT#ACL could be used if it enhanced with a subclass that implements Structure.ByReference
There was a problem hiding this comment.
I created a PACLByReference class, similar to PSIDByReference. Embedding support into ACL opened up a can of worms based on how that class is structured.
| * function fails, the return value is zero. For extended error | ||
| * information, call GetLastError. | ||
| */ | ||
| boolean GetAce(ACL pAcl, int dwAceIndex, PointerByReference pAce); |
There was a problem hiding this comment.
Could ACCESS_ACEStructure be used and would it be sensible? (maye also 530)
There was a problem hiding this comment.
That structure is abstract, so while we could define the function to take it, users would need to use their own implementation or pass in a ACCESS_ALLOWED_ACE or ACCESS_DENIED_ACE instead. I did try this, passing an instance of ACCESS_ALLOWED_ACE in, but now the new write() overload crashes from autoWrite because the psid member is null. Actually, given this find, I should probably do something about null pointer.
|
Dang it, something happened with that last commit. Too many changes. I'll see what I can do to roll that back. |
|
This should be in better shape. Let me know if I should do something about |
…to mod-security
|
This looks good overall - I'd like to understand this though: + int sidLength = Advapi32.INSTANCE.GetLengthSid(pSid);
+ cbAcl = Native.getNativeSize(ACL.class, null);
+ cbAcl += Native.getNativeSize(ACCESS_ALLOWED_ACE.class, null);
+ cbAcl += (sidLength - DWORD.SIZE);
+ cbAcl = Advapi32Util.alignOnDWORD(cbAcl);
+ pAcl = new ACL(cbAcl);The alignment is a requirement of InitializeAcl (so far so good), the size is calculated from an empty ACL, ACE and a SID that is refercend and that is aligned on a DWORD boundary. But why the subtraction on the sidLength?! |
|
ACCESS_ALLOWED_ACE contains DWORD SidStart (via its parent class) and so Native.getNativeSize(ACCESS_ALLOWED_ACE.class, null); includes the size of that member. sidLength is the entire length, including these 4 bytes as well. So in order not to double count those 4 bytes, subtract the DWORD.SIZE (aka SidStart's size.) There is an example here. |
|
Thank you for the explanation and example. I think this is the last suggestion I have: --- a/contrib/platform/src/com/sun/jna/platform/win32/Advapi32Util.java
+++ b/contrib/platform/src/com/sun/jna/platform/win32/Advapi32Util.java
@@ -403,18 +403,6 @@
- DWORD.SIZE;
}
- /**
- * Get the first 4 bytes of the PSID for a SidStart
- * @param psid the PSID
- * @return the SidStart
- * @throws IOException
- */
- public static int getSidStart(PSID psid) throws IOException {
- byte[] sidStart = psid.getBytes();
- DataInputStream dis = new DataInputStream(new ByteArrayInputStream(sidStart));
- return dis.readInt();
- }
-
/**
* Get an account name from a string SID on the local machine.
*
diff --git a/contrib/platform/src/com/sun/jna/platform/win32/WinNT.java b/contrib/platform/src/com/sun/jna/platform/win32/WinNT.java
index f4bfe3a..63fe682 100644
--- a/contrib/platform/src/com/sun/jna/platform/win32/WinNT.java
+++ b/contrib/platform/src/com/sun/jna/platform/win32/WinNT.java
@@ -2758,57 +2758,32 @@
* ACCESS_ALLOWED_ACE and ACCESS_DENIED_ACE have the same structure layout
*/
public static abstract class ACCESS_ACEStructure extends ACEStructure {
- public static final List<String> EXTRA_ABSTRACT_FIELDS = createFieldsOrder("Mask", "SidStart");
- private static final AtomicReference<List<String>> fieldsHolder = new AtomicReference<List<String>>(null);
- private static List<String> resolveEffectiveFields(List<String> baseFields) {
- List<String> fields;
- synchronized (fieldsHolder) {
- fields = fieldsHolder.get();
- if (fields == null) {
- fields = createFieldsOrder(baseFields, EXTRA_ABSTRACT_FIELDS);
- fieldsHolder.set(fields);
- }
- }
-
- return fields;
- }
+ public static final List<String> FIELDS = createFieldsOrder(ACEStructure.FIELDS, "Mask", "SidStart");
public int Mask;
- /**
- * first 4 bytes of the SID
- */
- public DWORD SidStart;
+
+ // Only used to hava a valid field defined - use sid!
+ public byte[] SidStart = new byte[4];
public ACCESS_ACEStructure() {
super();
}
- public ACCESS_ACEStructure(int Mask, int SidStart, byte AceType, byte AceFlags, short AceSize, PSID psid) {
+ public ACCESS_ACEStructure(int Mask, byte AceType, byte AceFlags, PSID psid) {
super();
- this.allocateMemory(AceSize);
+ this.calculateSize(true);
this.AceType = AceType;
this.AceFlags = AceFlags;
- this.AceSize = AceSize;
+ this.AceSize = (short) (super.fieldOffset("SidStart") + psid.getBytes().length);
this.psid = psid;
this.Mask = Mask;
- this.SidStart = new DWORD(SidStart);
+ this.allocateMemory(AceSize);
write();
}
public ACCESS_ACEStructure(Pointer p) {
super(p);
read();
- // Check for AceSize being zero, can happen on empty memory
- if (AceSize != 0) {
- int sizeOfSID = super.AceSize - size() + 4;
- // ACE_HEADER + size of int (Mask)
- int offsetOfSID = 4 + 4;
- byte[] data = p.getByteArray(offsetOfSID, sizeOfSID);
- psid = new PSID(data);
- }
- else {
- psid = null;
- }
}
/**
@@ -2816,21 +2791,33 @@
*/
@Override
public void write() {
- int sizeOfSID = super.AceSize - 8;
- int offsetOfSID = 4 + 4;
- super.writeField("AceType");
- super.writeField("AceFlags");
- super.writeField("AceSize");
- super.writeField("Mask");
- // Get bytes from the PSID
- byte[] psidWrite = psid.getPointer().getByteArray(0, sizeOfSID);
- // Write those bytes to native memory
- super.getPointer().write(offsetOfSID, psidWrite, 0, sizeOfSID);
+ super.write();
+ int offsetOfSID = super.fieldOffset("SidStart");
+ int sizeOfSID = super.AceSize - super.fieldOffset("SidStart");
+ if(psid != null) {
+ // Get bytes from the PSID
+ byte[] psidWrite = psid.getBytes();
+ assert psidWrite.length <= sizeOfSID;
+ // Write those bytes to native memory
+ getPointer().write(offsetOfSID, psidWrite, 0, sizeOfSID);
+ }
}
@Override
+ public void read() {
+ super.read();
+ int offsetOfSID = super.fieldOffset("SidStart");
+ int sizeOfSID = super.AceSize - super.fieldOffset("SidStart");
+ if(sizeOfSID > 0) {
+ psid = new PSID(getPointer().getByteArray(offsetOfSID, sizeOfSID));
+ } else {
+ psid = new PSID();
+ }
+ }
+
+ @Override
protected List<String> getFieldOrder() {
- return resolveEffectiveFields(super.getFieldOrder());
+ return FIELDS;
}
}
@@ -2844,8 +2831,8 @@
super(p);
}
- public ACCESS_ALLOWED_ACE(int Mask, int SidStart, byte AceFlags, short AceSize, PSID psid) {
- super(Mask, SidStart, ACCESS_ALLOWED_ACE_TYPE, AceFlags, AceSize, psid);
+ public ACCESS_ALLOWED_ACE(int Mask, byte AceFlags, PSID psid) {
+ super(Mask, ACCESS_ALLOWED_ACE_TYPE, AceFlags, psid);
}
}
diff --git a/contrib/platform/test/com/sun/jna/platform/win32/Advapi32Test.java b/contrib/platform/test/com/sun/jna/platform/win32/Advapi32Test.java
old mode 100755
new mode 100644
index f51e35b..4a42285
--- a/contrib/platform/test/com/sun/jna/platform/win32/Advapi32Test.java
+++ b/contrib/platform/test/com/sun/jna/platform/win32/Advapi32Test.java
@@ -834,19 +834,15 @@
int sidLength = Advapi32.INSTANCE.GetLengthSid(pSid);
cbAcl = Native.getNativeSize(ACL.class, null);
- cbAcl += Native.getNativeSize(ACCESS_ALLOWED_ACE.class, null);
- cbAcl += (sidLength - DWORD.SIZE);
+ cbAcl += Advapi32Util.getAceSize(sidLength);
cbAcl = Advapi32Util.alignOnDWORD(cbAcl);
pAcl = new ACL(cbAcl);
- int cbAce = Advapi32Util.getAceSize(sidLength);
ACCESS_ALLOWED_ACE pace = new ACCESS_ALLOWED_ACE(WinNT.STANDARD_RIGHTS_ALL,
- Advapi32Util.getSidStart(pSid),
WinNT.INHERITED_ACE,
- (short)cbAce,
pSid);
assertTrue(Advapi32.INSTANCE.InitializeAcl(pAcl, cbAcl, WinNT.ACL_REVISION));
- assertTrue(Advapi32.INSTANCE.AddAce(pAcl, WinNT.ACL_REVISION, WinNT.MAXDWORD, pace.getPointer(), cbAce));
+ assertTrue(Advapi32.INSTANCE.AddAce(pAcl, WinNT.ACL_REVISION, WinNT.MAXDWORD, pace.getPointer(), pace.size()));
PointerByReference pAce = new PointerByReference(new Memory(16));
assertTrue(Advapi32.INSTANCE.GetAce(pAcl, 0, pAce));
@@ -865,8 +861,7 @@
int sidLength = Advapi32.INSTANCE.GetLengthSid(pSid);
cbAcl = Native.getNativeSize(ACL.class, null);
- cbAcl += Native.getNativeSize(ACCESS_ALLOWED_ACE.class, null);
- cbAcl += (sidLength - DWORD.SIZE);
+ cbAcl += Advapi32Util.getAceSize(sidLength);
cbAcl = Advapi32Util.alignOnDWORD(cbAcl);
pAcl = new ACL(cbAcl);
assertTrue(Advapi32.INSTANCE.InitializeAcl(pAcl, cbAcl, WinNT.ACL_REVISION));
@@ -889,8 +884,7 @@
int sidLength = Advapi32.INSTANCE.GetLengthSid(pSid);
cbAcl = Native.getNativeSize(ACL.class, null);
- cbAcl += Native.getNativeSize(ACCESS_ALLOWED_ACE.class, null);
- cbAcl += (sidLength - DWORD.SIZE);
+ cbAcl += Advapi32Util.getAceSize(sidLength);
cbAcl = Advapi32Util.alignOnDWORD(cbAcl);
pAcl = new ACL(cbAcl);
assertTrue(Advapi32.INSTANCE.InitializeAcl(pAcl, cbAcl, WinNT.ACL_REVISION));I think this makes the ACE structures better handable. |
|
@matthiasblaesing, I made the changes. However, before you accept them, I noticed that testAddAce is hanging on the AddAce call. I'm trying to figure out why that is. If I revert to the prior commit it works. I will let you know. |
Also set the SidStart bytes value in the relevant constructor
|
I figured out the problem. I missed the parent class write() call, so the memory wasn't being set. Also, I did add setting the SidStart value in that one constructor just for consistency with the older versions, otherwise it was all zeros. |
|
@amarcionek Thank you - merged. |
Motivation: We are behind on the java patch releases used during build Modifications: Bump up java versions in docker-compose files Result: Use latest java versions
Advapi32
NtDll