Adding security functions and RtlNtStatusToDosError to NtDll by amarcionek · Pull Request #738 · java-native-access/jna

amarcionek · 2016-12-02T17:11:19Z

Advapi32

GetSecurityDescriptorOwner
SetSecurityDescriptorOwner
GetSecurityDescriptorGroup
SetSecurityDescriptorGroup
GetSecurityDescriptorControl
SetSecurityDescriptorControl
GetSecurityDescriptorDacl
SetSecurityDescriptorDacl
MakeSelfRelativeSD
MakeAbsoluteSD
EqualSid
InitializeSecurityDescriptor
InitializeAcl
AddAce
AddAccessAllowedAce
AddAccessAllowedAceEx
GetAce

NtDll

RtlNtStatusToDosError

* 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

matthiasblaesing · 2016-12-02T19:40:05Z

+     *         If the SID structures are not equal, the return value is zero. To get extended error
+     *         information, call GetLastError.
+     *         If either SID structure is not valid, the return value is undefined.
+     */


Is there a difference between pSid1.equals(pSid2) and EqualSid(pSid1, pSid2)? The definition of Structure#equals looks reasonable.

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.

Sorry - I read outdated documentation. dataEquals does the "right thing" anyways - the function is small simple documentated, so lets keep it.

matthiasblaesing · 2016-12-02T19:45:42Z

+     * @return If the function succeeds, the return value is nonzero. If the
+     *         function fails, the return value is zero. For extended error
+     *         information, call GetLastError.
+     */


pControl is a PSECURITY_DESCRIPTOR_CONTROL is defined as:

typedef WORD SECURITY_DESCRIPTOR_CONTROL, *PSECURITY_DESCRIPTOR_CONTROL;

Shouldn't this be ShortByReference?

matthiasblaesing · 2016-12-02T19:47:27Z

+     * @return If the function succeeds, the return value is nonzero. If the
+     *         function fails, the return value is zero. For extended error
+     *         information, call GetLastError.
+     */


See comment on 331 - ControlBitsOfInterest and ControlBitsToSet are defined to be word-size.

matthiasblaesing · 2016-12-02T19:48:30Z

+     * @return If the function succeeds, the return value is nonzero. If the
+     *         function fails, the return value is zero. For extended error
+     *         information, call GetLastError.
+     */


There is a PSIDByReference already in the codebase would that be sensible here for pOwner?

matthiasblaesing · 2016-12-02T19:50:41Z

+     * @return If the function succeeds, the return value is nonzero. If the
+     *         function fails, the return value is zero. For extended error
+     *         information, call GetLastError.
+     */


see comment on GetSecurityDescriptorOwner here for pGroup.

matthiasblaesing · 2016-12-02T19:55:15Z

+     * @return If the function succeeds, the return value is nonzero. If the
+     *         function fails, the return value is zero. For extended error
+     *         information, call GetLastError.
+     */


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

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.

matthiasblaesing · 2016-12-02T19:58:42Z

+     * @return If the function succeeds, the return value is nonzero. If the
+     *         function fails, the return value is zero. For extended error
+     *         information, call GetLastError.
+     */


Could ACCESS_ACEStructure be used and would it be sensible? (maye also 530)

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.

amarcionek · 2016-12-05T19:48:43Z

Dang it, something happened with that last commit. Too many changes. I'll see what I can do to roll that back.

amarcionek · 2016-12-05T20:20:53Z

This should be in better shape. Let me know if I should do something about ACCESS_ACEStructure#write. It didn't occur to me that write() could be called at other places (autoWrite, for example. I sort of assumed it would only from the new constructors that assign psid to a value.

…to mod-security

matthiasblaesing · 2016-12-06T19:43:28Z

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

amarcionek · 2016-12-06T20:25:19Z

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.

matthiasblaesing · 2016-12-07T19:56:27Z

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.

amarcionek · 2016-12-12T16:15:04Z

@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

amarcionek · 2016-12-12T16:57:40Z

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.

matthiasblaesing · 2016-12-12T20:51:53Z

@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

amarcionek added 6 commits November 28, 2016 14:08

Added functions to Advapi32

7dbaf77

* EqualSid * InitializeSecurityDescriptor * InitializeAcl * AddAce * AddAccessAllowedAce * AddAccessAllowedAceEx * GetAce Added other ways to manipulate and initialize ACL and ACE in WinNT

Added functions to Advapi32

c79c266

* GetSecurityDescriptorControl * SetSecurityDescriptorControl * GetSecurityDescriptorDacl * SetSecurityDescriptorDacl * MakeSelfRelativeSD * MakeAbsoluteSD

Fixed javadoc

434bda3

Added functions to Advapi32

99abc8e

* GetSecurityDescriptorOwner * SetSecurityDescriptorOwner * GetSecurityDescriptorGroup * SetSecurityDescriptorGroup

Added RtlNtStatusToDosError to NtDll

6fa6406

Updated for changes for PR 738

c34e519

matthiasblaesing reviewed Dec 2, 2016

View reviewed changes

Changes based on feedback

e03e269

Merge branch 'master' of https://github.com/java-native-access/jna in…

7b67784

…to mod-security

Cleaned up ACCESS_ACEStructure

c3e5b09

Fixed incorrect structure write override

922dad1

Also set the SidStart bytes value in the relevant constructor

matthiasblaesing merged commit 82c8638 into java-native-access:master Dec 12, 2016

amarcionek deleted the mod-security branch June 17, 2019 17:59

Conversation

amarcionek commented Dec 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amarcionek commented Dec 5, 2016

Uh oh!

amarcionek commented Dec 5, 2016

Uh oh!

matthiasblaesing commented Dec 6, 2016

Uh oh!

amarcionek commented Dec 6, 2016

Uh oh!

matthiasblaesing commented Dec 7, 2016

Uh oh!

amarcionek commented Dec 12, 2016

Uh oh!

amarcionek commented Dec 12, 2016

Uh oh!

matthiasblaesing commented Dec 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants