[Esapi-user] Esapi-user Digest, Vol 9, Issue 6

Owen Berger owen.k.berger at gmail.com
Sat Aug 7 18:21:50 EDT 2010


There is a difference between a bad List being passed, and the
ESAPI.properties being unavailable, unreadable, or tainted, right?

It seems that because there could be two different implementations,
there should be two different functions. There are two different ways
for the allowedExtensions list to be null or invalid, and that is 1)
if an invalid List is passed, or 2) if the List retrieved by
securityConfiguration() is unavailable or invalid.

Therefore, I think that there should be two different methods, one
that gets a user supplied list, and the other that gets the list from
securityConfiguration().  The reason for this separation is that one
should throw a ValidationException, the other should throw an
AvailabilityException, or just exit with a RuntimeException because it
will mean that there is something wrong with ESAPI.properties.

-Owen

Jim Manico wrote:
>* How about*>* *>* if ((allowedExtensions == null) || (allowedExtensions.isEmpty())) {*>* 	throw new ValidationException( "Internal Error", "You called*>* getValidFileName with an empty or null list of allowed Extensions, therefore*>* no files can be uploaded" );*>* }*
This one, but I suggest for the logged message part,

	getValidFileName called with an empty ...

rather than "You called getValidFileName with an empty ..."

I like this better than RuntimeException. (Unless you want to make
it IllegalArgumentException, which *is* a RuntimeException. But I like
this better since you already need to catch ValidationExeption.)

-kevin
-- 
Kevin W. Wall
"The most likely way for the world to be destroyed, most experts agree,
is by accident. That's where we come in; we're computer professionals.
We cause accidents."        -- Nathaniel Borenstein, co-creator of MIME


> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of Esapi-user digest..."
>
>
> Today's Topics:
>
>   1. Re: getValidFile  bug (ESAPI 2.0, at least) (Jim Manico)
>   2. Re: getValidFile  bug (ESAPI 2.0, at least) (Kevin W. Wall)
>   3. Re: getValidFile  bug (ESAPI 2.0, at least) (Jim Manico)
>   4. Re: getValidFile  bug (ESAPI 2.0, at least) (Jim Manico)
>   5. Re: getValidFile  bug (ESAPI 2.0, at least) (Kevin W. Wall)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Wed, 4 Aug 2010 13:04:55 -1000
> From: "Jim Manico" <jim.manico at owasp.org>
> Subject: Re: [Esapi-user] getValidFile  bug (ESAPI 2.0, at least)
> To: <esapi-user at lists.owasp.org>,       "'ESAPI-Developers'"
>        <esapi-dev at lists.owasp.org>
> Message-ID: <4c59f21f.d99ce60a.3804.103a at mx.google.com>
> Content-Type: text/plain; charset="us-ascii"
>
> I see a bug in the getValidFile validator function. The error message for
> getValidFileName should not use the ESAPI configured extension exclusion
> list (underlined below), but instead should use the allowedExtensions
> function augment. Fair?
>
>
>
>      public String getValidFileName(String context, String input,
> List<String> allowedExtensions, boolean allowNull) throws
> ValidationException, IntrusionException {
>
>            String canonical = "";
>
>            // detect path manipulation
>
>            try {
>
>                  if (isEmpty(input)) {
>
>                        if (allowNull) return null;
>
>                        throw new ValidationException( context + ": Input
> file name required", "Input required: context=" + context + ", input=" +
> input, context );
>
>                  }
>
>
>
>                  // do basic validation
>
>              canonical = new File(input).getCanonicalFile().getName();
>
>              getValidInput( context, input, "FileName", 255, true );
>
>
>
>                  File f = new File(canonical);
>
>                  String c = f.getCanonicalPath();
>
>                  String cpath = c.substring(c.lastIndexOf(File.separator) +
> 1);
>
>
>
>
>
>                  // the path is valid if the input matches the canonical
> path
>
>                  if (!input.equals(cpath)) {
>
>                        throw new ValidationException( context + ": Invalid
> file name", "Invalid directory name does not match the canonical path:
> context=" + context + ", input=" + input + ", canonical=" + canonical,
> context );
>
>                  }
>
>
>
>            } catch (IOException e) {
>
>                  throw new ValidationException( context + ": Invalid file
> name", "Invalid file name does not exist: context=" + context + ",
> canonical=" + canonical, e, context );
>
>            }
>
>
>
>            // verify extensions
>
>            Iterator<String> i = allowedExtensions.iterator();
>
>            while (i.hasNext()) {
>
>                  String ext = i.next();
>
>                  if (input.toLowerCase().endsWith(ext.toLowerCase())) {
>
>                        return canonical;
>
>                  }
>
>            }
>
>            throw new ValidationException( context + ": Invalid file name
> does not have valid extension (
> "+ESAPI.securityConfiguration().getAllowedFileExtensions()+")", "Invalid
> file name does not have valid extension (
> "+ESAPI.securityConfiguration().getAllowedFileExtensions()+"): context=" +
> context+", input=" + input, context );
>
>      }
>
>
>
>
>
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:
> https://lists.owasp.org/pipermail/esapi-user/attachments/20100804/537bef31/attachment-0001.html
>
> ------------------------------
>
> Message: 2
> Date: Wed, 04 Aug 2010 20:01:35 -0400
> From: "Kevin W. Wall" <kevin.w.wall at gmail.com>
> Subject: Re: [Esapi-user] getValidFile  bug (ESAPI 2.0, at least)
> To: Jim Manico <jim.manico at owasp.org>
> Cc: 'ESAPI-Developers' <esapi-dev at lists.owasp.org>,
>        esapi-user at lists.owasp.org
> Message-ID: <4C59FF5F.7050401 at gmail.com>
> Content-Type: text/plain; charset=ISO-8859-1
>
> Jim Manico wrote:
> > I see a bug in the getValidFile validator function. The error message
> > for getValidFileName should not use the ESAPI configured extension
> > exclusion list (underlined below), but instead should use the
> > allowedExtensions function augment. Fair?
>
> Agreed. And while you're making the change, might I suggest that
> you add a few lines of code to treat a null or empty allowedExtensions
> list to be treated as using the _default_ file extensions from
> ESAPI.properties. E.g., add this
>
>        if ( allowedExtensions == null || allowedExtensions.size() == 0 ) {
>                allowedExtensions =
>                   ESAPI.securityConfiguration().getAllowedFileExtensions();
>        }
>
> since otherwise someone passing a null or empty list makes no sense and
> we should check for that anyway. And note as appropriate in the method's
> Javadoc as well.
>
> -kevin
> --
> Kevin W. Wall
> "The most likely way for the world to be destroyed, most experts agree,
> is by accident. That's where we come in; we're computer professionals.
> We cause accidents."        -- Nathaniel Borenstein, co-creator of MIME
>
>
> ------------------------------
>
> Message: 3
> Date: Wed, 04 Aug 2010 14:02:39 -1000
> From: Jim Manico <jim.manico at owasp.org>
> Subject: Re: [Esapi-user] getValidFile  bug (ESAPI 2.0, at least)
> To: "Kevin W. Wall" <kevin.w.wall at gmail.com>
> Cc: 'ESAPI-Developers' <esapi-dev at lists.owasp.org>,
>        esapi-user at lists.owasp.org
> Message-ID: <4C59FF9F.4040206 at owasp.org>
> Content-Type: text/plain; charset=ISO-8859-1; format=flowed
>
>  I'd rather throw a Runtime. Acceptable?
> - Jim
>
> > Jim Manico wrote:
> >> I see a bug in the getValidFile validator function. The error message
> >> for getValidFileName should not use the ESAPI configured extension
> >> exclusion list (underlined below), but instead should use the
> >> allowedExtensions function augment. Fair?
> > Agreed. And while you're making the change, might I suggest that
> > you add a few lines of code to treat a null or empty allowedExtensions
> > list to be treated as using the _default_ file extensions from
> > ESAPI.properties. E.g., add this
> >
> >       if ( allowedExtensions == null || allowedExtensions.size() == 0 ) {
> >               allowedExtensions =
> >
>  ESAPI.securityConfiguration().getAllowedFileExtensions();
> >       }
> >
> > since otherwise someone passing a null or empty list makes no sense and
> > we should check for that anyway. And note as appropriate in the method's
> > Javadoc as well.
> >
> > -kevin
>
>
>
> ------------------------------
>
> Message: 4
> Date: Wed, 4 Aug 2010 14:07:00 -1000
> From: "Jim Manico" <jim.manico at owasp.org>
> Subject: Re: [Esapi-user] getValidFile  bug (ESAPI 2.0, at least)
> To: "'Kevin W. Wall'" <kevin.w.wall at gmail.com>
> Cc: 'ESAPI-Developers' <esapi-dev at lists.owasp.org>,
>        esapi-user at lists.owasp.org
> Message-ID: <4c5a00ab.4c30dc0a.4838.ffffe468 at mx.google.com>
> Content-Type: text/plain;       charset="us-ascii"
>
> How about
>
> if ((allowedExtensions == null) || (allowedExtensions.isEmpty())) {
>        throw new ValidationException( "Internal Error", "You called
> getValidFileName with an empty or null list of allowed Extensions,
> therefore
> no files can be uploaded" );
> }
>
> Or
>
> if ((allowedExtensions == null) || (allowedExtensions.isEmpty())) {
>        throw new RuntimeException("You called getValidFileName with an
> empty or null list of allowed Extensions, therefore no files can be
> uploaded" );
> }
>
> ?
> -----Original Message-----
> From: Kevin W. Wall [mailto:kevin.w.wall at gmail.com]
> Sent: Wednesday, August 04, 2010 2:02 PM
> To: Jim Manico
> Cc: esapi-user at lists.owasp.org; 'ESAPI-Developers'
> Subject: Re: [Esapi-user] getValidFile bug (ESAPI 2.0, at least)
>
> Jim Manico wrote:
> > I see a bug in the getValidFile validator function. The error message
> > for getValidFileName should not use the ESAPI configured extension
> > exclusion list (underlined below), but instead should use the
> > allowedExtensions function augment. Fair?
>
> Agreed. And while you're making the change, might I suggest that
> you add a few lines of code to treat a null or empty allowedExtensions
> list to be treated as using the _default_ file extensions from
> ESAPI.properties. E.g., add this
>
>        if ( allowedExtensions == null || allowedExtensions.size() == 0 ) {
>                allowedExtensions =
>                   ESAPI.securityConfiguration().getAllowedFileExtensions();
>        }
>
> since otherwise someone passing a null or empty list makes no sense and
> we should check for that anyway. And note as appropriate in the method's
> Javadoc as well.
>
> -kevin
> --
> Kevin W. Wall
> "The most likely way for the world to be destroyed, most experts agree,
> is by accident. That's where we come in; we're computer professionals.
> We cause accidents."        -- Nathaniel Borenstein, co-creator of MIME
>
>
>
> ------------------------------
>
> Message: 5
> Date: Wed, 04 Aug 2010 22:09:43 -0400
> From: "Kevin W. Wall" <kevin.w.wall at gmail.com>
> Subject: Re: [Esapi-user] getValidFile  bug (ESAPI 2.0, at least)
> To: Jim Manico <jim.manico at owasp.org>
> Cc: 'ESAPI-Developers' <esapi-dev at lists.owasp.org>,
>        esapi-user at lists.owasp.org
> Message-ID: <4C5A1D67.5090404 at gmail.com>
> Content-Type: text/plain; charset=ISO-8859-1
>
> Jim Manico wrote:
> > How about
> >
> > if ((allowedExtensions == null) || (allowedExtensions.isEmpty())) {
> >       throw new ValidationException( "Internal Error", "You called
> > getValidFileName with an empty or null list of allowed Extensions,
> therefore
> > no files can be uploaded" );
> > }
>
> This one, but I suggest for the logged message part,
>
>        getValidFileName called with an empty ...
>
> rather than "You called getValidFileName with an empty ..."
>
> I like this better than RuntimeException. (Unless you want to make
> it IllegalArgumentException, which *is* a RuntimeException. But I like
> this better since you already need to catch ValidationExeption.)
>
> -kevin
> --
> Kevin W. Wall
> "The most likely way for the world to be destroyed, most experts agree,
> is by accident. That's where we come in; we're computer professionals.
> We cause accidents."        -- Nathaniel Borenstein, co-creator of MIME
>
>
> ------------------------------
>
> _______________________________________________
> Esapi-user mailing list
> Esapi-user at lists.owasp.org
> https://lists.owasp.org/mailman/listinfo/esapi-user
>
>
> End of Esapi-user Digest, Vol 9, Issue 6
> ****************************************
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.owasp.org/pipermail/esapi-user/attachments/20100807/48001584/attachment.html 


More information about the Esapi-user mailing list