Velocity Tools
  1. Velocity Tools
  2. VELTOOLS-150

VelocityLayoutServlet allows clients to specify "layout" without performing any security checks.

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: 1.4, 2.0
    • Fix Version/s: None
    • Component/s: VelocityView
    • Labels:
    • Environment:
      Velocity 1.7, Velocity Tools 2.0.
      Confirmed also affects Velocity 1.4, Velocity Tools 1.4.

      Activity

      Christopher Schultz created issue -
      Hide
      Christopher Schultz added a comment -

      I see us having several options, here:

      1. Disable this feature
      2. Make this feature optional and configurable, with the default being /disabled/
      3. Lock-down the process that allows certain paths to protect the webapp when this feature /is/ used

      I think that #2 is a good idea in general: I suspect that most people don't actually use this feature, so disabling it will certainly eliminate this attack vector.

      #3 might be touchy, since any file in a webapp - not just in WEB-INF or META-INF - could potentially be sensitive. It's a reasonable assumption that things in WEB-INF and META-INF should be protected by this particular feature, but it might not be straightforward since the "layout" directory is relative to the webapp, and then the layout selected by the request parameter will be relative to that. We may have to normalize the path and then compare it to known "sensitive" path prefixes. I'm not sure how to get the container to normalize a path for us, though. Maybe we just need to look for ".." in the layout name and ignore anything that looks like that. Suggestions are certainly welcome.

      Certainly, templates or servlets, etc. themselves need to be exempt from these measures in case programmers want to use templates that are outside the norm: these security rules should probably only be applied when the layout is being selected from the request parameters. Request attributes, for instance, should be considered trusted.

      Show
      Christopher Schultz added a comment - I see us having several options, here: 1. Disable this feature 2. Make this feature optional and configurable, with the default being /disabled/ 3. Lock-down the process that allows certain paths to protect the webapp when this feature /is/ used I think that #2 is a good idea in general: I suspect that most people don't actually use this feature, so disabling it will certainly eliminate this attack vector. #3 might be touchy, since any file in a webapp - not just in WEB-INF or META-INF - could potentially be sensitive. It's a reasonable assumption that things in WEB-INF and META-INF should be protected by this particular feature, but it might not be straightforward since the "layout" directory is relative to the webapp, and then the layout selected by the request parameter will be relative to that. We may have to normalize the path and then compare it to known "sensitive" path prefixes. I'm not sure how to get the container to normalize a path for us, though. Maybe we just need to look for ".." in the layout name and ignore anything that looks like that. Suggestions are certainly welcome. Certainly, templates or servlets, etc. themselves need to be exempt from these measures in case programmers want to use templates that are outside the norm: these security rules should probably only be applied when the layout is being selected from the request parameters. Request attributes, for instance, should be considered trusted.
      Hide
      Nathan Bubna added a comment -

      Ok, while this still feels like something that ought to live at the ResourceLoader level, i accept that adding a little security to the VLS is not that hard. I think we should check paths for ".." and ignore such requests. Do you want to do the fix Christopher?

      Show
      Nathan Bubna added a comment - Ok, while this still feels like something that ought to live at the ResourceLoader level, i accept that adding a little security to the VLS is not that hard. I think we should check paths for ".." and ignore such requests. Do you want to do the fix Christopher?
      Hide
      Christopher Schultz added a comment -

      Sure, I can do a simple fix like that.

      I think I'd also like to introduce a configuration setting that allows this feature to be disabled entirely. While I'd prefer to leave it disabled by default, it might actually break someone's webapp so we should probably wait for another major release before making that kind of change.

      Show
      Christopher Schultz added a comment - Sure, I can do a simple fix like that. I think I'd also like to introduce a configuration setting that allows this feature to be disabled entirely. While I'd prefer to leave it disabled by default, it might actually break someone's webapp so we should probably wait for another major release before making that kind of change.
      Hide
      Nathan Bubna added a comment -

      Agreed on all points.

      Show
      Nathan Bubna added a comment - Agreed on all points.
      Hide
      Christopher Schultz added a comment -

      Okay, I have gotten back into this and I have a couple new thoughts:

      1. Resource Loaders may not be able to protect against this kind of stuff, because a typical deployment may have "webapp.resource.loader.path=/" and it's not unheard of to place files like *.jsp and *.vm inside the /WEB-INF directory to keep them otherwise hidden.

      2. VelocityLayoutServlet treats layout paths as String values and only the ResourceLoader may convert that into a path-like thing to load it. The ResourceLoader doesn't know that the VelocityLayoutServlet is trying to load a template (from a "templates" directory which should have some of the checking we're discussing here). Therefore, VelocityLayoutServlet really would need to do its own validation that the layout template is in fact inside the layout path.

      3. Checking paths is easy if you are using the file system (java.io.File) but not straightforward when the resources are stored in a WAR file, etc. Thus, I don't really think that any checking can practically be done... any such definite checking would have to be storage-specific and probably expensive in terms of performance (e.g. using java.io.File to canonicalize two paths for every request).

      I have a patch that simply makes the current behavior optional, with the default being to disallow the request parameters from being able to override the layout. I think we might want to leave it at that for the time being.

      Show
      Christopher Schultz added a comment - Okay, I have gotten back into this and I have a couple new thoughts: 1. Resource Loaders may not be able to protect against this kind of stuff, because a typical deployment may have "webapp.resource.loader.path=/" and it's not unheard of to place files like *.jsp and *.vm inside the /WEB-INF directory to keep them otherwise hidden. 2. VelocityLayoutServlet treats layout paths as String values and only the ResourceLoader may convert that into a path-like thing to load it. The ResourceLoader doesn't know that the VelocityLayoutServlet is trying to load a template (from a "templates" directory which should have some of the checking we're discussing here). Therefore, VelocityLayoutServlet really would need to do its own validation that the layout template is in fact inside the layout path. 3. Checking paths is easy if you are using the file system (java.io.File) but not straightforward when the resources are stored in a WAR file, etc. Thus, I don't really think that any checking can practically be done... any such definite checking would have to be storage-specific and probably expensive in terms of performance (e.g. using java.io.File to canonicalize two paths for every request). I have a patch that simply makes the current behavior optional, with the default being to disallow the request parameters from being able to override the layout. I think we might want to leave it at that for the time being.

        People

        • Assignee:
          Unassigned
          Reporter:
          Christopher Schultz
        • Votes:
          0 Vote for this issue
          Watchers:
          1 Start watching this issue

          Dates

          • Created:
            Updated:

            Development