AV bei LookupAccountSidW

[Olli]

Hier mal etwas menschenlesbarer

Code:

.text:7C924CE1 ; int __stdcall RtlCopyUnicodeString(PUNICODE_STRING DestinationString,PUNICODE_STRING SourceString)
.text:7C924CE1                 public _RtlCopyUnicodeString@8
.text:7C924CE1 _RtlCopyUnicodeString@8 proc near       ; CODE XREF: RtlConvertSidToUnicodeString(x,x,x)+158p
.text:7C924CE1                                         ; LdrLoadAlternateResourceModule(x,x)+34A22p ...
.text:7C924CE1
.text:7C924CE1 DestinationString= dword ptr  8
.text:7C924CE1 SourceString    = dword ptr  0Ch
.text:7C924CE1
.text:7C924CE1 ; FUNCTION CHUNK AT .text:7C934947 SIZE 00000014 BYTES
.text:7C924CE1
.text:7C924CE1                 mov     edi, edi
.text:7C924CE3                 push    ebp
.text:7C924CE4                 mov     ebp, esp
.text:7C924CE6                 mov     eax, [ebp+SourceString]
.text:7C924CE9                 test    eax, eax        ; Test pointer
.text:7C924CEB                 jz      BailOut01
.text:7C924CF1                 mov     edx, [ebp+DestinationString]
.text:7C924CF4 ; CX = DestinationString.MaximumLength
[color=blue].text:7C924CF4                 mov     cx, [edx+UNICODE_STRING.MaximumLength][/color]
.text:7C924CF8                 push    ebx
.text:7C924CF9                 push    esi
.text:7C924CFA ; ESI = SourceString.Buffer
.text:7C924CFA                 mov     esi, [eax+UNICODE_STRING.Buffer]
.text:7C924CFD ; EAX = SourceString.Length
.text:7C924CFD                 movzx   eax, [eax+UNICODE_STRING.Length]
.text:7C924D00 ; if(AX > CX) ...
.text:7C924D00                 cmp     ax, cx          ; Does it fit?
.text:7C924D03                 push    edi
[color=red].text:7C924D04                 mov     edi, [edx+UNICODE_STRING.Buffer][/color]
.text:7C924D07                 mov     [ebp+DestinationString], edi ; "Misuse" the stack location ;)
.text:7C924D0A                 ja      TruncateToMaxLength
.text:7C924D10
.text:7C924D10 BackFromTruncate:                       ; CODE XREF: RtlCopyUnicodeString(x,x)+FC69j
.text:7C924D10                 mov     ecx, eax        ; EAX contains number of bytes
.text:7C924D12                 mov     ebx, ecx        ; Save for later (ECX gets decremented)
.text:7C924D14                 shr     ecx, 2          ; Divide by sizeof(DWORD)
.text:7C924D17 ; Now store either the Length of SourceString or MaxLength
.text:7C924D17 ; of DestinationString into DestinationString.Length
[color=darkgreen].text:7C924D17                 mov     [edx+UNICODE_STRING.Length], ax[/color]
[color=red].text:7C924D1A                 rep movsd               ; Copy DWORD-wise into buffer @EDI[/color]
.text:7C924D1C                 mov     ecx, ebx        ; Restore size in Bytes into ECX
.text:7C924D1E                 and     ecx, 3          ; Remainder from SHR
.text:7C924D21                 rep movsb               ; Copy remaining Bytes (byte-wise)
.text:7C924D23                 mov     cx, [edx+UNICODE_STRING.Length]
.text:7C924D26                 cmp     cx, [edx+UNICODE_STRING.MaximumLength]
.text:7C924D2A                 pop     edi             ; Restore from 7C924D03
.text:7C924D2B                 pop     esi             ; Restore from 7C924CF9
.text:7C924D2C                 pop     ebx             ; Restore from 7C924CF8
.text:7C924D2D                 jnb     short ExitThis
.text:7C924D2F ; Zero-terminate DestinationString only if we have
.text:7C924D2F ; enough space. Otherwise get out.
.text:7C924D2F                 mov     ecx, [ebp+DestinationString]
.text:7C924D32                 shr     eax, 1          ; Divide by 2 (== sizeof WCHAR)
.text:7C924D34                 and     word ptr [ecx+eax*2], 0 ; Zero-terminate
.text:7C924D39
.text:7C924D39 ExitThis:                               ; CODE XREF: RtlCopyUnicodeString(x,x)+4Cj
.text:7C924D39                                         ; RtlCopyUnicodeString(x,x)+FC75j
.text:7C924D39                 pop     ebp
.text:7C924D3A                 retn    8
.text:7C924D3A _RtlCopyUnicodeString@8 endp

An 7C924D1A wird EDI gesetzt also der Zielpuffer. Die Stackposition des übergebenen Parameters wird dazu temporär "mißbraucht", nicht wundern, das ist normal (macht der Compiler).

Ich bitte festzuhalten, daß in der blau markierten Zeile bei dem 2ten Aufruf von Dezi's Testprogramm bereits einmal von Adresse 2 (nil+2) gelesen wird (formal PUNICODE_STRING(p)^.MaximumLength, also 2 Bytes/1 Word). In der ersten roten Zeile wird von Adresse 4 (nil+4) gelesen (formal PUNICODE_STRING(p)^.Buffer, also 4 Bytes/1 DWORD).

Und jetzt kommt der wunde Punkt. Wieso knallt's in der zweiten roten Zeile und nicht bereits in der Zeile davor (grün), wo ja bereits an 0 (nil) geschrieben wird (formal PUNICODE_STRING(p)^.Length, also 2 Bytes)? Könnte was mit Speicheralignment zu tun haben, vielleicht ist aber einfach die Beobachtung von Dezi falsch oder falsch interpretiert?!

Da (p == nil), folgt:

PUNICODE_STRING(nil)^.Length := Irgendwas; ... das kann man in Delphi, soweit ich mich entsinne, auch exakt so ausdrücken und kompilieren!!!

Das würde in jeder Hinsicht knallen. Am Anfang der Funktion wird auch nur SourceString überprüft, jedoch nicht DestinationString - was theoretisch bereits in Bailout01 schiefgehen sollte!

Code:

.text:7C93494F BailOut01:                              ; CODE XREF: RtlCopyUnicodeString(x,x)+Aj
.text:7C93494F                 mov     eax, [ebp+DestinationString]
[color=red].text:7C934952                 and     [eax+UNICODE_STRING.Length], 0[/color]
.text:7C934956                 jmp     ExitThis
.text:7C934956 ; END OF FUNCTION CHUNK FOR _RtlCopyUnicodeString@8

Eine Übergabe von nil für beide Parameter sollte also genauso zum Absturz führen, da hier wiederum an Adresse 0 (nil) geschrieben wird (rote Zeile, zwei Bytes, entsrpricht der grünen Zeile von oben).

[Luckie]

Aber hätte das Microsoft nicht bei der Qualitätskontrolle auffallen müssen?

[Dezipaitor]

Welche QM ?

[Christian Seehase]

Moin Zusammen,

also ich sehe es jetzt nicht gerade als ein Problem an, dass die W-Funktion nicht sauber durchläuft, wenn sie mit ungültigen Parametern aufgerufen wird, sondern dass die A-Variante es tut.
Die W-Variante müsste natürlich, sinnvoller Weise, 87 zurückmelden, statt auf eine AV aufzulaufen, aber den eigentlichen Fehler sehe ich in der anderen Version, da die Parameter, zumindest gemäss Doku, so nicht nicht zulässig sind.

[Olli]

Christian, das sehe ich ganz ähnlich. Zumal es sinnlos ist RtlCopyUnicodeString mit nur einem (gültigen) Parameter aufzurufen. Die einzige Sache welche mich daran stört ist, daß der eine Parameter (nämlich SourceString) durchaus überprüft wird (siehe 7C924CE9), so daß es irgendwie auch wieder inkonsequent ist den zweiten Parameter nicht auch zu überprüfen.

Auf der anderen Seite muß man es abwägen, da zuviel Fehlerüberprüfung Performance kostet. Formal müßte man zum komplettieren ja auch überprüfen ob der Pointer gültig ist (nicht nur ob er ungleich nil ist) und ob man mindestens sizeof(UNICODE_STRING) lesen/schreiben kann und ob das auch für den Pointer in us.Buffer gilt.

[Christian Seehase]

Moin Olli,

Zitat von Olli:

Auf der anderen Seite muß man es abwägen, da zuviel Fehlerüberprüfung Performance kostet.

Wobei mangelnde Fehlerprüfung zu Sicherheitslücken führen kann.
Da ist mir die Vorabprüfung aber lieber

[Dezipaitor]

wobei man hätte die Überprüfung auch bei
LookupAccountSidW
einbauen können.

[Olli]

Frage noch immer offen?

[Dezipaitor]

Die Frage ist offener als ein Scheunentor groß sein kann.

Ich darf mal das Szenario nachstellen :

Microsoft macht in etwa das:

Code:

CSidData::InternalLookUpSids(..., ISecurityInformation2 pS2, ...);
{
  ..
  ..
  LPDataObject lpDO = NULL;
  ...
  HRESULT hr = pS2->LookUpSids(..., lpDO); //mein Aufruf OK
  if (hr <> S_OK)
    FAIL

  hr = lpDO->GetData(....);   //exception hier ( call dword ptr [ecx+$0c] )
  if (hr <> S_OK) 
    FAIL
  ...
}

Es sieht so aus, als ob die Speicherstelle bei [ecx+$0c] garnicht mein GetData ist.
Der Vergleich von @GetData mit der Stelle ergibt über $300-$1000 Bytes Unterschied.

Ich bin echt ratlos, besonders da eine reine C++ Implementierung einwandfrei funktioniert.
Auch eine komplette neue Implementation des SecurityDialogs unter Delphi mit
sowenig Code wie möglich, läuft auf dasselbe hinaus.

Es scheint als hätte Borland einen totalen Sc*****d**** gemacht - oder es liegt an was anderes.
Auf jeden Fall bin ich verzweifelt - soetwas hab ich noch nie erlebt.

---

EDIT: Fehler im Code korrigiert.

[Christian Seehase]

Moin Dezipaitor,

hast Du mal in Erwägung gezogen, die benötigten Funktionen (und ggf. Strukturen) selber zu deklarieren?

Im Gegensatz zu Borland, importiere ich die Funktionen immer so, dass ich alle Parameter als const deklariere, und dann, bei den var Parametern gezielt Pointer übergebe, da diese Vorgehensweise eher dem entspricht, was man dann auch in Beispielen findet.
Ausserdem kann man bei einem var-Parameter auch nicht nil übergeben, was ja bei vielen Funktionen durchaus zulässig ist.