microsoft / go-winio

Win32 IO-related utilities for Go
MIT License
946 stars 180 forks source link

backuptar: SecurityDescriptorFromTarHeader() don't decode twice #233

Closed thaJeztah closed 2 years ago

thaJeztah commented 2 years ago

I noticed this when looking at #220

While writing, I wondered why we didn't just return the result of base64.StdEncoding.DecodeString() directly, but it looks like that may return "garbage" output on failure; https://github.com/golang/go/blob/go1.17.7/src/encoding/base64/base64.go#L382-L387

Here's a short example showing that; https://go.dev/play/p/6kCw85ofiTx

package main

import (
    "encoding/base64"
    "fmt"
)

func main() {
    sd1, err := base64.StdEncoding.DecodeString("Hello, 世界")
    if err != nil {
        fmt.Println(err)
    }
    fmt.Printf("%+v: %s\n", sd1, string(sd1))
}

The above shows;

illegal base64 data at input byte 5
[29 233 101]: �e

While callers should never trust output if an error occurs, it's probably better to explicitly return nil just to be sure no garbage is used.


This may be a theoretical situation, but the tar headers may contain both the old and new headers, in which case we would be decoding both.

This patch rewrites the function to try the new headers first, and return early if found, then fall back to trying the old headers.

thaJeztah commented 2 years ago

@ambarve @kevpar ptal

thaJeztah commented 2 years ago

Thanks for reviewing, everyone!