Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: image: support decoder instance with specific formats #20814

Closed
epelc opened this issue Jun 27, 2017 · 9 comments
Closed

proposal: image: support decoder instance with specific formats #20814

epelc opened this issue Jun 27, 2017 · 9 comments

Comments

@epelc
Copy link

@epelc epelc commented Jun 27, 2017

Today canonical way to register formats for the image package is to import a package which calls image.RegisterFormat. This is simple and works fine for small programs however when your programs start to grow there is no way to ensure that the image.Decode function will only attempt to decode a predefined list of formats.

Example scenario

You have a profile picture system which you want to accept jpeg, and png but you also have an admin page which requires reading tiff images. The tiff image package is still experimental and you would prefer not to expose it to external users who just need to upload a profile picture in common formats.

Right now the only way to prevent your profile picture upload handler from using the tiff package is to reimplement a large part of https://golang.org/src/image/format.go. The only other option is to write separate programs which may or may not make sense or be easy depending on your ops.

In short this is a security(limiting exposure to certain image decoders) and developer usability issue.

Required additions/API

The required additions aren't very large but the api is important since this will be here for a while(go1 compat). The real issue is not implementing this, it is agreeing upon how this should work and look. The following is a rough outline for what would need to be added.

  • Add a new Decoder type similar to json.Decoder which can accept a list of formats to look for and accept.
  • Make definitions for params commonly passed into image.RegisterFormat publicly available in image decoder packages
    • Maybe make image.format public so that each image decoder package could define

Possible api and usage

// package image
type Decoder struct {
  formats []format
}

func NewDecoder(formats []Format) *Decoder {
 return &Decoder{formats: formats}
}

func (d *Decoder) Decode() {
  // Implement decode using `d.formats`
}

Changes required for image format package(image/png, image/jpeg, etc). Replaces init() with something like this https://golang.org/src/image/png/reader.go#L1017

// package image/png

// Format defines the format for a png image.
// You can use this when creating your own image.Decoder instance.
var Format = image.Format{"png", pngHeader, Decode, DecodeConfig}

func init() {
  // Register Format in the package level decoder so that we keep the go1 compatibility promise
  Format.Register() // calls image.RegisterFormat with `Format` as the data
}

Usage for a user wanting to only use specific formats without relying on all other packages to not import other image format packages which automatically register themselves on the package level decoder.

// package myapp/handlers

func SaveUserUpload(r io.Reader) {
  dec := image.NewDecoder([]image.Format{png.Format, jpeg.Format})
  // Only accepts and looks for `png` and `jpeg`
  dec.Decode(r)
}
@gopherbot gopherbot added this to the Proposal milestone Jun 27, 2017
@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Jul 6, 2017

"Right now the only way to prevent your profile picture upload handler from using the tiff package is to reimplement a large part of https://golang.org/src/image/format.go."

Well, format.go is only 100 lines of code, including comments, so a large part of that is still a small amount of code. If I was writing a program that should decode only JPEG and PNG but not TIFF, then I'd just do that. Or even just hard code a 10 line function that's: switch on the first byte, case 0x89: try png.Decode, case 0xff: try jpeg.Decode.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Jul 17, 2017

zip does this with RegisterDecompressor (global) vs Reader.RegisterDecompressor (local to Reader), but there's no equivalent of Reader yet in the image API. We'd have to add one.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Jul 17, 2017

Maybe we should consider this at the same time as #20804, #8055, #5050 and maybe more specific options like #13614, #18365.

Loading

@epelc
Copy link
Author

@epelc epelc commented Jul 17, 2017

@rsc the zip package looks like a good example except for this behaviour. Defaulting to looking up package defaults ruins the increased security you'd get from manually registering image decoders. It might also be something to look at in the zip package as another issue.

https://golang.org/pkg/archive/zip/#Reader.RegisterDecompressor

If a decompressor for a given method is not found, Reader will default to looking up the decompressor at the package level.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Jul 17, 2017

Honestly I'm kind of skeptical of the "increased security". If you really want to decode only a few formats, you can sniff the input types. Registering alternate decoders seems more useful. Also, if you really do want the security you could register a decoder with magic="".

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Oct 9, 2017

@nigeltao or @robpike, any thoughts about a path forward here?

Loading

@robpike
Copy link
Contributor

@robpike robpike commented Oct 10, 2017

I don't think there's a forward path I find valuable. The current situation is fine with me and the problem seems minor and easy to work around with the existing API.

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Oct 12, 2017

I agree with @robpike.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Feb 5, 2018

Based on the discussion above, this seems to have limited value and is possible to do as a helper package outside the standard library. I'd encourage anyone interested to start by doing that.

Loading

@rsc rsc closed this Feb 5, 2018
@golang golang locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants