DefaultValidator.getValidInput implementation ignores 'canonicalize' method parameter #476

Activity

1. jeremiahjstacey commented on Jan 31, 2019

   

   jeremiahjstacey

   on Jan 31, 2019

   CollaboratorAuthor

   More actions

   This comment includes original feedback from @kwwall for additional context/elaboration on issue description

   I think the correct way forward here probably is:

   1. Create a new GitHub issue that notes that

        Default.getValidInput(String context, String input, String
      

   type, int maxLength, boolean allowNull, boolean canonicalize) ALWAYS
   ignores 'canonicalize' flag

   2. Fix this by changing that method to:

      public String getValidInput(String context, String input, String
      type, int maxLength, boolean allowNull, boolean canonicalize) throws
      ValidationException {
      StringValidationRule rvr = new StringValidationRule( type, encoder );
      Pattern p = ESAPI.securityConfiguration().getValidationPattern( type );
      if ( p != null ) {
      rvr.addWhitelistPattern( p );
      } else {
      // Issue 232 - Specify requested type in exception message - CS
      throw new IllegalArgumentException("The selected type [" +
      type + "] was not set via the ESAPI validation configuration");
      }
      rvr.setMaximumLength(maxLength);
      rvr.setAllowNull(allowNull);
      rvr.setCanonicalize(canonicalize); // New; issue # xxx
      return rvr.getValid(context, input);
      }

   and rewriting StringValidationRule to have a cannonicalize boolean field that is
   set to 'true' by default and also with 'final' setter / getter methods, and
   then change StringValidationRule.getValid() to ONLY call

        data = encoder.canonicalize( input );
   

   if the 'canonicalize' boolean is true.

   Furthermore, if the 'canonicalize' flag is false, I also would be very tempted
   to add a call to Logger.warn() with the event type of SECURITY_AUDIT
   (SECURITY_FAIL may be pushing it, since it would be intentional) to log a
   audit warning since generally it is NOT safe to validate uncanonicalized data.

   3. Add new test cases to test this.

   Note that one reason that I would prefer the StringValidationRule to handle the
   canonicalization (versus, say pushing it to the base class, BaseValidationRule)
   because if you look at the various other DefaultValidator methods such as
   getValidCreditCard() which uses other BaseValidationRule subclasses, they
   explicitly state that canonicalization will be done, so why even make it
   possible to get it wrong in these other use cases?

   I don't know if that gives you enough to go on or not, but I think doing this
   will have the minimum impact.

   My logic for the Logger.warn() in StringValidationRule.getValid() when someone
   has called
   StringValidationRule.setCanonicalize( false );
   if because that way we can (sort of, if the pay attention to their logs) warn
   any who are already directly calling
   Default.getValidInput(context, input, type, maxLength, allowNull, false)

   and have simply forgotten they were doing so because canonicalization
   was always being enforced. I figure even though we would not
   technically be "breaking" things, changing this could still cause
   problems and I generally think it is a bad think not to canonicalize
   before validation, so logging a warning seems like a reasonable middle
   ground to me.

   Does that make sense to you? If not, let me know where they gaps in
   your understanding are and I will try to clarify.

   -kevin
2. jeremiahjstacey commented on Feb 1, 2019

   

   jeremiahjstacey

   on Feb 1, 2019

   CollaboratorAuthor

   More actions

   @kwwall @xeno6696 This functionality was actually removed since the 2.1.0.1 release.

   The behavior was originally introduced 8 years ago:

   commit 0867375
   Validate the string both before and after canonicalization. Previously
   it was only validated after. This also adds a
   setValidateInputAndCanonical(boolean) method that, when passed false,
   will make StringValidationRule behave as before.

   To simplify this a fair bit was refactored as well.

   This closes issue 54.

   It was removed last year:

   commit a9849f1
   Issue #284 -- Restored original canonicalization behavior due to issue #54
   being an invalid fix for a legacy URL issue where query params would get
   flagged as HTML Entities.

   From these commits, I would like to ask if we should re-assess the proposed solution. It sounds like doing this would re-introduce issue 284?
3. xeno6696 commented on Feb 1, 2019

   

   xeno6696

   on Feb 1, 2019

   Collaborator

   More actions

   So the only thing #284 changed was that we were validating before and after canonicalization. I don’t see you restoring that behavior here.
4. kwwall commented on Feb 2, 2019

   

   kwwall

   on Feb 2, 2019

   Contributor

   More actions

   Agree with @xeno6696 here. There was no reason to do validation twice and this proposed change is not restoring that behavior. I think you can go ahead and fix it as you mentioned and as we discussed in email and make a PR and I will make sure we get it merged.
5. kwwall

   added a commit that references this issue on Feb 2, 2019

   #476 DefaultValidator.getValidInput uses canonicalize method argument (…

   ...

   3475e86
6. kwwall commented on Feb 2, 2019

   

   kwwall

   on Feb 2, 2019

   Contributor

   More actions

   Closed via PR #477
7. kwwall

   closed this as completedon Feb 2, 2019