Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Interchange>
  3. users
Re: [interchange] Revert "Add image file check mechanism to verify file type before passing to"
mike at heins
May 14, 2016, 5:28 AM
Post #1 of 9 (2418 views)
Permalink
On Fri, May 13, 2016 at 3:23 PM, David Christensen <
interchange-cvs@icdevgroup.org> wrote:

> commit c5ee9b0690521ccf54c6a186b9841a868a018b92
> Author: David Christensen <david@endpoint.com>
> Date: Fri May 13 14:22:48 2016 -0500
>
> Revert "Add image file check mechanism to verify file type before
> passing to"
>
> Per discussion, this is not Interchange's responsibility.
>
>
Since the image tag does call "mogrify", I would argue that it is the Image
tag's responsibility.


> This reverts commit 68d34396df797232e005e5bd164a1b8e72779bb6.
>
> code/SystemTag/image.tag | 29 +++--------------------------
> 1 files changed, 3 insertions(+), 26 deletions(-)
> ---
> diff --git a/code/SystemTag/image.tag b/code/SystemTag/image.tag
> index f7ec938..6d4de2a 100644
> --- a/code/SystemTag/image.tag
> +++ b/code/SystemTag/image.tag
> @@ -9,15 +9,13 @@ UserTag image Order src
> UserTag image AttrAlias geometry makesize
> UserTag image AttrAlias resize makesize
> UserTag image AddAttr
> -UserTag image Version 1.27
> +UserTag image Version 1.26
> UserTag image Routine <<EOR
> sub {
> my ($src, $opt) = @_;
> my ($image, $path, $secure, $sku);
> my ($imagedircurrent, $imagedir, $imagedirsecure);
>
> - use Image::Size;
> -
> my @descriptionfields = grep /\S/, split /\s+/,
> $opt->{descriptionfields} ||
> $::Variable->{DESCRIPTIONFIELDS} || $Vend::Cfg->{DescriptionField};
> @descriptionfields = qw( description ) if ! @descriptionfields;
> @@ -30,22 +28,6 @@ sub {
> my $filere = qr/\.\w{2,4}$/;
> my $absurlre = qr!^(?i:https?)://!;
>
> - my $verify_image = sub {
> - my $file = shift;
> -
> - return unless -f $file;
> -
> - my ($imgx, $imgy, $error) = imgsize($file);
> -
> - if(! $imgx) {
> - ::logError("Image::Size error on verify-image:
> $error");
> - return undef;
> - }
> -
> - return 1 if ($error =~
> /(jpg|jpeg|gif|png|bmp|tif|ico|xbm)/i);
> - return undef;
> - };
> -
> if ($opt->{ui}) {
> # unless no image dir specified, add locale string
> my $locale = $Scratch->{mv_locale} ? $Scratch->{mv_locale}
> : 'en_US';
> @@ -240,12 +222,6 @@ sub {
> }
> }
> last MOGIT unless $exec;
> -
> - unless ($verify_image->($newpath)){
> - logError("Image file not valid
> image:%s", $newpath);
> - last MOGIT;
> - }
> -
> system qq{$exec -geometry "$siz"
> '$newpath'};
> if($?) {
> logError("%s: Unable to mogrify
> image '%s'", 'image tag', $newpath);
> @@ -265,7 +241,8 @@ sub {
>
> if ($opt->{getsize} and $path) {
> eval {
> - my ($width, $height) = imgsize($path);
> + require Image::Size;
> + my ($width, $height) =
> Image::Size::imgsize($path);
> $opt->{height} = $height
> if defined($height) and not
> exists($opt->{height});
> $opt->{width} = $width
>
> _______________________________________________
> interchange-cvs mailing list
> interchange-cvs@icdevgroup.org
> http://www.icdevgroup.org/mailman/listinfo/interchange-cvs
>



--
The problem with Internet quotations is that many of them
are not genuine. -- Abraham Lincoln
Re: [interchange] Revert "Add image file check mechanism to verify file type before passing to" [ In reply to ]
david at endpoint
May 14, 2016, 7:05 AM
Post #2 of 9 (2392 views)
Permalink
> On May 14, 2016, at 7:28 AM, Mike Heins <mike@heins.com> wrote:
>
>> Per discussion, this is not Interchange's responsibility.
>>
> Since the image tag does call "mogrify", I would argue that it is the Image tag's responsibility.

Anyone who would update Interchange from git to fix this would already have the chops to fix the root problem anyway. This is an education/awareness issue, not something we should be working around. We aren't rolling our own TLS layer to fix Heartbleed, for instance. Why is this any different?

David
--
David Christensen
End Point Corporation
david@endpoint.com
785-727-1171




_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users
Re: [interchange] Revert "Add image file check mechanism to verify file type before passing to" [ In reply to ]
mikeh at endpoint
May 14, 2016, 7:37 AM
Post #3 of 9 (2394 views)
Permalink
Quoting David Christensen (david@endpoint.com):
>
> > On May 14, 2016, at 7:28 AM, Mike Heins <mike@heins.com> wrote:
> >
> >> Per discussion, this is not Interchange's responsibility.
> >>
> > Since the image tag does call "mogrify", I would argue that it is the Image tag's responsibility.
>
> Anyone who would update Interchange from git to fix this would already
> have the chops to fix the root problem anyway. This is an
> education/awareness issue, not something we should be working around.
> We aren't rolling our own TLS layer to fix Heartbleed, for instance.
> Why is this any different?

Because it makes sense, for all sorts of data integrity reasons, to limit
a program's input to that which it is intended to service. It is true that
the spur is a security issue, but the end is noble in and of itself.

The only downside would be a limitation of the program, which might be
able to handle unanticipated image types, but at this point the universe
of those types is pretty static.

--
Mike Heins
End Point -- Expert Internet Consulting http://www.endpoint.com/
phone +1.765.253.4194 <mikeh@endpoint.com>

Experience is what allows you to recognize a mistake the second
time you make it. -- unknown

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users
Re: [interchange] Revert "Add image file check mechanism to verify file type before passing to" [ In reply to ]
mike at heins
May 14, 2016, 8:10 AM
Post #4 of 9 (2392 views)
Permalink
Quoting Mike Heins (mikeh@endpoint.com):
> Quoting David Christensen (david@endpoint.com):
> >
> > > On May 14, 2016, at 7:28 AM, Mike Heins <mike@heins.com> wrote:
> > >
> > >> Per discussion, this is not Interchange's responsibility.
> > >>
> > > Since the image tag does call "mogrify", I would argue that it is the Image tag's responsibility.
> >
> > Anyone who would update Interchange from git to fix this would already
> > have the chops to fix the root problem anyway. This is an
> > education/awareness issue, not something we should be working around.
> > We aren't rolling our own TLS layer to fix Heartbleed, for instance.
> > Why is this any different?
>
> Because it makes sense, for all sorts of data integrity reasons, to limit
> a program's input to that which it is intended to service. It is true that
> the spur is a security issue, but the end is noble in and of itself.
>
> The only downside would be a limitation of the program, which might be
> able to handle unanticipated image types, but at this point the universe
> of those types is pretty static.

I guess, also, that it would mean that Image::Size is required for the
use of ImageMagick, but that seems to be a minor and managable dependency.
Image::Size is part of Bundle Interchange these days.

--
Mike Heins
End Point -- Expert Internet Consulting http://www.endpoint.com/
phone +1.765.253.4194 <mikeh@endpoint.com>

The problem with Internet quotations is that many of them
are not genuine. -- Abraham Lincoln

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users
Re: [interchange] Revert "Add image file check mechanism to verify file type before passing to" [ In reply to ]
david at endpoint
May 14, 2016, 8:15 AM
Post #5 of 9 (2394 views)
Permalink
> On May 14, 2016, at 9:37 AM, Mike Heins <mikeh@endpoint.com> wrote:
>
>>>> Per discussion, this is not Interchange's responsibility.
>>>>
>>> Since the image tag does call "mogrify", I would argue that it is the Image tag's responsibility.
>>
>> Anyone who would update Interchange from git to fix this would already
>> have the chops to fix the root problem anyway. This is an
>> education/awareness issue, not something we should be working around.
>> We aren't rolling our own TLS layer to fix Heartbleed, for instance.
>> Why is this any different?
>
> Because it makes sense, for all sorts of data integrity reasons, to limit
> a program's input to that which it is intended to service. It is true that
> the spur is a security issue, but the end is noble in and of itself.

> The only downside would be a limitation of the program, which might be
> able to handle unanticipated image types, but at this point the universe
> of those types is pretty static.

The patch as written was broken and caused image.tag to not compile when missing Image::Size (which is apparently only in Bundle::InterchangeKitchenSink); in any case, the demo site broke because it did not have this module. This was the motivator for the investigation and further discussion around how (or if) to fix this. (Had this not been the case, I likely would have ignored the patch. :-))

I think the ecosystem of various parts here is too large to take this approach with everything; *particularly* since it’s a separate external program, I think it’s the external tool’s responsibility to vet its own input. If we were using low-level graphics libraries inside Perl to handle all the manipulations or even the Image::Magick module directly (again, within Perl) I would say our code has a responsibility to check things like return codes, formats, etc. But it just seems like we’re committing to more than we should at this point.

David
--
David Christensen
End Point Corporation
david@endpoint.com
785-727-1171




_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users
Re: [interchange] Revert "Add image file check mechanism to verify file type before passing to" [ In reply to ]
mikeh at endpoint
May 14, 2016, 8:27 AM
Post #6 of 9 (2395 views)
Permalink
Quoting David Christensen (david@endpoint.com):
>
> > On May 14, 2016, at 9:37 AM, Mike Heins <mikeh@endpoint.com> wrote:
> >
> >>>> Per discussion, this is not Interchange's responsibility.
> >>>>
> >>> Since the image tag does call "mogrify", I would argue that it is the Image tag's responsibility.
> >>
> >> Anyone who would update Interchange from git to fix this would already
> >> have the chops to fix the root problem anyway. This is an
> >> education/awareness issue, not something we should be working around.
> >> We aren't rolling our own TLS layer to fix Heartbleed, for instance.
> >> Why is this any different?
> >
> > Because it makes sense, for all sorts of data integrity reasons, to limit
> > a program's input to that which it is intended to service. It is true that
> > the spur is a security issue, but the end is noble in and of itself.
>
> > The only downside would be a limitation of the program, which might be
> > able to handle unanticipated image types, but at this point the universe
> > of those types is pretty static.
>
> The patch as written was broken and caused image.tag to not compile
> when missing Image::Size (which is apparently only in
> Bundle::InterchangeKitchenSink);

No, it is in Bundle::Interchange and has been for a very long
time. I could see it not being installed on some systems due to
lack of libraries.

> in any case, the demo site broke
> because it did not have this module. This was the motivator for the
> investigation and further discussion around how (or if) to fix
> this. (Had this not been the case, I likely would have ignored the
> patch. :-))
>
> I think the ecosystem of various parts here is too large to take
> this approach with everything; *particularly* since it’s a
> separate external program, I think it’s the external tool’s
> responsibility to vet its own input. If we were using low-level
> graphics libraries inside Perl to handle all the manipulations or
> even the Image::Magick module directly (again, within Perl) I would
> say our code has a responsibility to check things like return codes,
> formats, etc. But it just seems like we’re committing to more than
> we should at this point.

I understand the viewpoint, and I am not insisting that we un-revert.
I am just speaking up for the other point of view.

--
Mike Heins
End Point -- Expert Internet Consulting http://www.endpoint.com/
phone +1.765.253.4194 <mikeh@endpoint.com>

Being against torture ought to be sort of a bipartisan thing.
-- Karl Lehenbauer

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users
Re: [interchange] Revert "Add image file check mechanism to verify file type before passing to" [ In reply to ]
david at endpoint
May 14, 2016, 8:35 AM
Post #7 of 9 (2393 views)
Permalink
> On May 14, 2016, at 10:27 AM, Mike Heins <mikeh@endpoint.com> wrote:
>>
>> The patch as written was broken and caused image.tag to not compile
>> when missing Image::Size (which is apparently only in
>> Bundle::InterchangeKitchenSink);
>
> No, it is in Bundle::Interchange and has been for a very long
> time. I could see it not being installed on some systems due to
> lack of libraries.

Fair enough; that was an assumption on my part as to why it was missing on the demo site.

>> in any case, the demo site broke
>> because it did not have this module. This was the motivator for the
>> investigation and further discussion around how (or if) to fix
>> this. (Had this not been the case, I likely would have ignored the
>> patch. :-))
>>
>> I think the ecosystem of various parts here is too large to take
>> this approach with everything; *particularly* since it’s a
>> separate external program, I think it’s the external tool’s
>> responsibility to vet its own input. If we were using low-level
>> graphics libraries inside Perl to handle all the manipulations or
>> even the Image::Magick module directly (again, within Perl) I would
>> say our code has a responsibility to check things like return codes,
>> formats, etc. But it just seems like we’re committing to more than
>> we should at this point.
>
> I understand the viewpoint, and I am not insisting that we un-revert.
> I am just speaking up for the other point of view.

Sounds good; I understand that viewpoint.
--
David Christensen
End Point Corporation
david@endpoint.com
785-727-1171




_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users
Re: [interchange] Revert "Add image file check mechanism to verify file type before passing to" [ In reply to ]
jbeima at palb
May 14, 2016, 8:55 AM
Post #8 of 9 (2393 views)
Permalink
Sent from my iPad

> On May 14, 2016, at 8:37 AM, Mike Heins <mikeh@endpoint.com> wrote:
>
> Quoting David Christensen (david@endpoint.com):
>>
>>>> On May 14, 2016, at 7:28 AM, Mike Heins <mike@heins.com> wrote:
>>>>
>>>> Per discussion, this is not Interchange's responsibility.
>>> Since the image tag does call "mogrify", I would argue that it is the Image tag's responsibility.
>>
>> Anyone who would update Interchange from git to fix this would already
>> have the chops to fix the root problem anyway. This is an
>> education/awareness issue, not something we should be working around.
>> We aren't rolling our own TLS layer to fix Heartbleed, for instance.
>> Why is this any different?
>
> Because it makes sense, for all sorts of data integrity reasons, to limit
> a program's input to that which it is intended to service. It is true that
> the spur is a security issue, but the end is noble in and of itself.
>
> The only downside would be a limitation of the program, which might be
> able to handle unanticipated image types, but at this point the universe
> of those types is pretty static.
>
> --
> Mike Heins
> End Point -- Expert Internet Consulting http://www.endpoint.com/
> phone +1.765.253.4194 <mikeh@endpoint.com>
>
> Experience is what allows you to recognize a mistake the second
> time you make it. -- unknown
>
> _______________________________________________
> interchange-users mailing list
> interchange-users@icdevgroup.org
> http://www.icdevgroup.org/mailman/listinfo/interchange-users

Could I chime in on this one?

Would it also not prevent Image Tragic?

By sanitizing the data?

Another point may be, many applications now "sanitize" the data to prevent SQL injections. Could this not be a way to do that for other attacks?



_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users
Re: [interchange] Revert "Add image file check mechanism to verify file type before passing to" [ In reply to ]
mikeh at endpoint
May 14, 2016, 9:19 AM
Post #9 of 9 (2399 views)
Permalink
Quoting David Christensen (david@endpoint.com):
>
> > On May 14, 2016, at 10:27 AM, Mike Heins <mikeh@endpoint.com> wrote:
> >>
> >> The patch as written was broken and caused image.tag to not compile
> >> when missing Image::Size (which is apparently only in
> >> Bundle::InterchangeKitchenSink);
> >
> > No, it is in Bundle::Interchange and has been for a very long
> > time. I could see it not being installed on some systems due to
> > lack of libraries.
>
> Fair enough; that was an assumption on my part as to why it was missing on the demo site.
>

If it is breaking sites even though they don't use mogrify, that
is a problem that could and should be corrected.

--
Mike Heins
End Point -- Expert Internet Consulting http://www.endpoint.com/
phone +1.765.253.4194 <mikeh@endpoint.com>

Experience is what allows you to recognize a mistake the second time you
make it. -- unknown

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal