(PUP-837) FFI Puppet::Util::Windows::SID

 - FFI Win32 API functions IsValidSid, ConvertSidToStringSid,
   ConvertStringSidToSid and LocalFree
 - Unfortunately ConvertSidToStringSidW allocates a buffer and does not
   return it's length, therefore a new helper function has been added to
   FFI::Pointer that will return a string up to a maximum length. If the
   NULL terminator is encountered prior to the max size, no additional
   memory is read.  This is not ideal, but slightly more secure than the
   previous implementation, which picked a buffer size, read all memory
   to the buffer, then called .strip on what remained.  The helper
   function also imposes a maximum of 512 UTF16 characters (1024 bytes)
 - Presume a maximum SID string length of 184 characters when formatted
   per http://stackoverflow.com/a/1792930
 - Add additional FFI MemoryPointer aliases for :wchar, #read_wchar
 - Remove any dependenc on mixins Windows::Security, Windows::Memory or
   Windows::MSVCRT
 - Updated affected callsites in security.rb to properly use or create
   FFI::Pointer when dealing with IsValidSid and sid_ptr_to_string. Also
   updated a couple of existing "old school" calls that are expecting
   addresses instead of FFI::Pointer objects.  This is a partial
   refactor until work continues with making security FFI compatible.

lib/puppet/util/windows/api_types.rb

@@ -16,6 +16,7 @@ def attach_function_private(*args)
 
   class ::FFI::Pointer
     NULL_HANDLE = 0
+    NULL_TERMINATOR_WCHAR = 0
 
     def self.from_string_to_wide_string(str)
       str = Puppet::Util::Windows::String.wide_string(str)
@@ -38,12 +39,29 @@ def read_handle
       type_size == 4 ? read_uint32 : read_uint64
     end
 
+    alias_method :read_wchar, :read_uint16
+
     def read_wide_string(char_length)
       # char_length is number of wide chars (typically excluding NULLs), *not* bytes
       str = get_bytes(0, char_length * 2).force_encoding('UTF-16LE')
       str.encode(Encoding.default_external)
     end
 
+    def read_arbitrary_wide_string_up_to(max_char_length = 512)
+      # max_char_length is number of wide chars (typically excluding NULLs), *not* bytes
+      # use a pointer to read one UTF-16LE char (2 bytes) at a time
+      wchar_ptr = FFI::Pointer.new(:wchar, address)
+
+      # now iterate 2 bytes at a time until an offset lower than max_char_length is found
+      0.upto(max_char_length - 1) do |i|
+        if wchar_ptr[i].read_wchar == NULL_TERMINATOR_WCHAR
+          return read_wide_string(i)
+        end
+      end
+
+      read_wide_string(max_char_length)
+    end
+
     alias_method :write_dword, :write_uint32
   end
 
@@ -99,4 +117,5 @@ def read_wide_string(char_length)
 
   # 8 bits per byte
   FFI.typedef :uchar, :byte
+  FFI.typedef :uint16, :wchar
 end

lib/puppet/util/windows/security.rb

@@ -391,9 +391,11 @@ def add_access_allowed_ace(acl, mask, sid, inherit = nil)
     inherit ||= NO_INHERITANCE
 
     string_to_sid_ptr(sid) do |sid_ptr|
-      raise Puppet::Util::Windows::Error.new("Invalid SID") unless IsValidSid(sid_ptr)
+      if Puppet::Util::Windows::SID.IsValidSid(sid_ptr) == FFI::WIN32_FALSE
+        raise Puppet::Util::Windows::Error.new("Invalid SID")
+      end
 
-      unless AddAccessAllowedAceEx(acl, ACL_REVISION, inherit, mask, sid_ptr)
+      unless AddAccessAllowedAceEx(acl, ACL_REVISION, inherit, mask, sid_ptr.address)
         raise Puppet::Util::Windows::Error.new("Failed to add access control entry")
       end
     end
@@ -403,9 +405,11 @@ def add_access_denied_ace(acl, mask, sid, inherit = nil)
     inherit ||= NO_INHERITANCE
 
     string_to_sid_ptr(sid) do |sid_ptr|
-      raise Puppet::Util::Windows::Error.new("Invalid SID") unless IsValidSid(sid_ptr)
+      if Puppet::Util::Windows::SID.IsValidSid(sid_ptr) == FFI::WIN32_FALSE
+        raise Puppet::Util::Windows::Error.new("Invalid SID")
+      end
 
-      unless AddAccessDeniedAceEx(acl, ACL_REVISION, inherit, mask, sid_ptr)
+      unless AddAccessDeniedAceEx(acl, ACL_REVISION, inherit, mask, sid_ptr.address)
         raise Puppet::Util::Windows::Error.new("Failed to add access control entry")
       end
     end
@@ -458,13 +462,18 @@ def parse_dacl(dacl_ptr)
 
       case ace_type
       when ACCESS_ALLOWED_ACE_TYPE
-        sid_ptr = ace_ptr.unpack('L')[0] + 8 # address of ace_ptr->SidStart
-        raise Puppet::Util::Windows::Error.new("Failed to read DACL, invalid SID") unless IsValidSid(sid_ptr)
+
+        sid_ptr = FFI::Pointer.new(:pointer, ace_ptr.unpack('L')[0] + 8) # address of ace_ptr->SidStart
+        if Puppet::Util::Windows::SID.IsValidSid(sid_ptr) == FFI::WIN32_FALSE
+          raise Puppet::Util::Windows::Error.new("Failed to read DACL, invalid SID")
+        end
         sid = sid_ptr_to_string(sid_ptr)
         dacl.allow(sid, mask, ace_flags)
       when ACCESS_DENIED_ACE_TYPE
-        sid_ptr = ace_ptr.unpack('L')[0] + 8 # address of ace_ptr->SidStart
-        raise Puppet::Util::Windows::Error.new("Failed to read DACL, invalid SID") unless IsValidSid(sid_ptr)
+        sid_ptr = FFI::Pointer.new(:pointer, ace_ptr.unpack('L')[0] + 8) # address of ace_ptr->SidStart
+        if Puppet::Util::Windows::SID.IsValidSid(sid_ptr) == FFI::WIN32_FALSE
+          raise Puppet::Util::Windows::Error.new("Failed to read DACL, invalid SID")
+        end
         sid = sid_ptr_to_string(sid_ptr)
         dacl.deny(sid, mask, ace_flags)
       else
@@ -562,12 +571,12 @@ def get_security_descriptor(path)
         raise Puppet::Util::Windows::Error.new("Failed to get security information") unless rv == ERROR_SUCCESS
 
         begin
-          owner = sid_ptr_to_string(owner_sid.unpack('L')[0])
-          group = sid_ptr_to_string(group_sid.unpack('L')[0])
+          owner = sid_ptr_to_string(FFI::Pointer.new(:pointer, owner_sid.unpack('L')[0]))
+          group = sid_ptr_to_string(FFI::Pointer.new(:pointer, group_sid.unpack('L')[0]))
 
           control = FFI::MemoryPointer.new(:word, 1)
           revision = FFI::MemoryPointer.new(:dword, 1)
-          ffsd = FFI::Pointer.new(ppsd.unpack('L')[0])
+          ffsd = FFI::Pointer.new(:pointer, ppsd.unpack('L')[0])
 
           if GetSecurityDescriptorControl(ffsd, control, revision) == FFI::WIN32_FALSE
             raise Puppet::Util::Windows::Error.new("Failed to get security descriptor control")
@@ -623,8 +632,8 @@ def set_security_descriptor(path, sd)
               rv = SetSecurityInfo(handle,
                                    SE_FILE_OBJECT,
                                    flags,
-                                   ownersid,
-                                   groupsid,
+                                   ownersid.address,
+                                   groupsid.address,
                                    acl,
                                    nil)
               raise Puppet::Util::Windows::Error.new("Failed to set security information") unless rv == ERROR_SUCCESS

lib/puppet/util/windows/sid.rb

@@ -2,14 +2,8 @@
 
 module Puppet::Util::Windows
   module SID
-    require 'windows/security'
-    include ::Windows::Security
-
-    require 'windows/memory'
-    include ::Windows::Memory
-
-    require 'windows/msvcrt/string'
-    include ::Windows::MSVCRT::String
+    require 'ffi'
+    extend FFI::Library
 
     # missing from Windows::Error
     ERROR_NONE_MAPPED           = 1332
@@ -68,21 +62,35 @@ def sid_to_name(value)
       nil
     end
 
+    # http://stackoverflow.com/a/1792930 - 68 bytes, 184 characters in a string
+    MAXIMUM_SID_STRING_LENGTH = 184
+
     # Convert a SID pointer to a SID string, e.g. "S-1-5-32-544".
     def sid_ptr_to_string(psid)
-      sid_buf = 0.chr * 256
-      str_ptr = 0.chr * 4
-
-      raise Puppet::Util::Windows::Error.new("Invalid SID") unless IsValidSid(psid)
+      if ! psid.instance_of?(FFI::Pointer) || IsValidSid(psid) == FFI::WIN32_FALSE
+        raise Puppet::Util::Windows::Error.new("Invalid SID")
+      end
 
-      raise Puppet::Util::Windows::Error.new("Failed to convert binary SID") unless ConvertSidToStringSid(psid, str_ptr)
+      buffer_ptr = FFI::MemoryPointer.new(:pointer, 1)
 
       begin
-        strncpy(sid_buf, str_ptr.unpack('L')[0], sid_buf.size - 1)
-        sid_buf[sid_buf.size - 1] = 0.chr
-        return sid_buf.strip
+        if ConvertSidToStringSidW(psid, buffer_ptr) == FFI::WIN32_FALSE
+          raise Puppet::Util::Windows::Error.new("Failed to convert binary SID")
+        end
+
+        wide_string_ptr = buffer_ptr.read_pointer
+
+        if wide_string_ptr.null?
+          raise Puppet::Error.new("ConvertSidToStringSidW failed to allocate buffer for sid")
+        end
+
+        return wide_string_ptr.read_arbitrary_wide_string_up_to(MAXIMUM_SID_STRING_LENGTH)
       ensure
-        LocalFree(str_ptr.unpack('L')[0])
+        if ! wide_string_ptr.nil? && ! wide_string_ptr.null?
+          if LocalFree(wide_string_ptr.address) != FFI::Pointer::NULL_HANDLE
+            Puppet.debug "LocalFree memory leak"
+          end
+        end
       end
     end
 
@@ -91,16 +99,19 @@ def sid_ptr_to_string(psid)
     # Win32 APIs that expect a PSID, e.g. IsValidSid. The account for this
     # SID may or may not exist.
     def string_to_sid_ptr(string, &block)
-      sid_buf = 0.chr * 80
-      string_addr = [string].pack('p*').unpack('L')[0]
+      lpcwstr = FFI::MemoryPointer.from_string_to_wide_string(string)
+      sid_ptr_ptr = FFI::MemoryPointer.new(:pointer, 1)
 
-      raise Puppet::Util::Windows::Error.new("Failed to convert string SID: #{string}") unless ConvertStringSidToSid(string_addr, sid_buf)
+      if ConvertStringSidToSidW(lpcwstr, sid_ptr_ptr) == FFI::WIN32_FALSE
+        raise Puppet::Util::Windows::Error.new("Failed to convert string SID: #{string}")
+      end
 
-      sid_ptr = sid_buf.unpack('L')[0]
       begin
-        yield sid_ptr
+        yield sid_ptr = sid_ptr_ptr.read_pointer
       ensure
-        LocalFree(sid_ptr)
+        if LocalFree(sid_ptr.address) != FFI::Pointer::NULL_HANDLE
+          Puppet.debug "LocalFree memory leak"
+        end
       end
     end
 
@@ -114,5 +125,40 @@ def valid_sid?(string)
         raise
       end
     end
+
+    ffi_convention :stdcall
+
+    # http://msdn.microsoft.com/en-us/library/windows/desktop/aa379151(v=vs.85).aspx
+    # BOOL WINAPI IsValidSid(
+    #   _In_  PSID pSid
+    # );
+    ffi_lib :advapi32
+    attach_function_private :IsValidSid,
+      [:pointer], :win32_bool
+
+    # http://msdn.microsoft.com/en-us/library/windows/desktop/aa376399(v=vs.85).aspx
+    # BOOL ConvertSidToStringSid(
+    #   _In_   PSID Sid,
+    #   _Out_  LPTSTR *StringSid
+    # );
+    ffi_lib :advapi32
+    attach_function_private :ConvertSidToStringSidW,
+      [:pointer, :pointer], :win32_bool
+
+    # http://msdn.microsoft.com/en-us/library/windows/desktop/aa376402(v=vs.85).aspx
+    # BOOL WINAPI ConvertStringSidToSid(
+    #   _In_   LPCTSTR StringSid,
+    #   _Out_  PSID *Sid
+    # );
+    ffi_lib :advapi32
+    attach_function_private :ConvertStringSidToSidW,
+      [:lpcwstr, :pointer], :win32_bool
+
+    # http://msdn.microsoft.com/en-us/library/windows/desktop/aa366730(v=vs.85).aspx
+    # HLOCAL WINAPI LocalFree(
+    #   _In_  HLOCAL hMem
+    # );
+    ffi_lib :kernel32
+    attach_function_private :LocalFree, [:handle], :handle
   end
 end