Forum OpenACS Q&A: Security hole in ad_form (may change behavior of ad_form to fix!)

Derek Keller also noticed this, and it is discussed here:

https://openacs.org/bugtracker/openacs/bug?bug%5fnumber=899

See my comment at the bottom of the page for a link to the IRC chat where Jeff Davis suggests a solution.

The problem is that ad_form lets [ ] and $ be interpreted by the Tcl interpreter. This makes it trivial to break into an OpenACS website, or do a lot of damage.

I ran across this while coding the project-manager. I had a task named "My test task [edited]", and on a page which used a list of tasks in a select box, I received an error page that said, "no such command, "edited"".

So if I had said [rm -rf *] instead, it would have executed that command.

Lars' solution is okay, but it doesn't do it by default. I really think it should be, even if it causes things to break. It's just to serious to allow it to be a default option.

Since the fix will be in 5.0, and we're having to make changes to code to get it up to 5.0 anyway, we can adapt code to make it work. Security issues like this take precedence I think.

---------------------------------

Here's a test case Jeff Davis came up with:

proc YY {} { return {BADBAD} }
proc XX {} { return {DANGER[YY]} }

set zim {[XX]}
ad_form -name foo -export zim -form {
    {dependency_task_id.6595:text(select)
        {label "Dependency[XX]"}
        {options { {"One two three" 6193} {"Task Dos [XX]" [XX]} {"This is my task1" 6218} {"blah, blah" 5500} } }
        {value {}}
        {html {size[XX] [XX]}}
        {help_text {Task the dependency is based on [XX]}}}
}

----------------------------------------------------------------------
<master>
<property name="title">Foo</property>
<property name="context">"Foo test"</property>

<formtemplate id="foo"></formtemplate>

I should add that $ are a problem too. You can get errors if you include anything with a $ into an ad_form.

I'm lost, is the [ or $ passed in by the user or just in the call to ad_form. If this is just in ad_form, what is the issue?

Ok, what happens is this:

In order to let you declare your forms nicely with curly braces instead of having to use [list] and put backslashes at the end of each line, while still building a dynamic form, ad_form performs a [subst] on the right-hand side of, I believe, all its arguments.

For example in:

{label $my_label}

The literal string "$my_label" will get an [uplevel subst ...] performed on it by ad_form.

Thus if you let users contribute the content that goes on that right-hand side, they can potentially sneak in something that contains [evil_code] and evil_code will get executed.

However, if the local variable my_label above contains the literal string [evil_code], there'll be no problem, since it'll only get subst'ed once, replacing the literal string "$my_label" with the value of the my_label local variable, the literal string "[evil_code]".

The problem occurs when you construct a Tcl list in a local variable like this:

set elms [list]

foreach ... {
    lappend elms [list $name:text [list label $label]]
}

In this case, if the local variable "label" happens to contain $'s or ['s, stuff will get evaluated.

I think a safe solution is to do it like this instead:

foreach ... {
    ad_form -extend -name my_form -form {
        $name:text {label $label}
    }
}

Hm. Actually, on second thought, this WILL NOT work, because ad_form does NOT do a subst on the first part ($name:text) where you declare your widget and its name. But if it did, the above would work and be safe.

/Lars

When I wrote ad_form, my intention was that dynamic forms be built using -extend, so I personally prefer Lars's solution - do a subst on the form element name/type definition as well as its attributes.

Are there any objections to this?  It would only break existing code if folks named their forms using "$" or "[]", which I would describe as bad coding practice anyway.

As far as programmers including something dangerous themselves  directly in ad_form, this is no more a problem than it is with a set statement.  "set foo [exec rm -f]" is equally evil ...

So it is really just the double-substitution instance noted by Lars that is an issue, no?

So what will work is this:

foreach ... {
    ad_form -extend -name my_form -form \
        [list [list $name:text [list label \$label]]]
}

Since we include the literal string $label in the argument to ad_form, thanks to the backslash, the "$label" string will get subst'ed by ad_form, cauing the result to safely and reliably be what we want it to.

Sounds like something we need to document.

/Lars

Refering to Lars example, why not

[list [list \$name:text [list label \$label]]]

How is someone gonna remember when and where to use backslashes?

Don: subst couldn't be the same as set? Subst is dangerous on any user supplied code, and even the switches -nocommands -novariables -nobackslashes have limited effect on user supplied strings, unless you use all three. Examples:

subst -nocommands $a([exec rm -f *]) ;# still exec
subst -novariables a[myproc $a]b ;# still sets $a

So either subst is required, or it isn't. I think any fix should be heavily tested to ensure it does what is expected and doesn't allow arbitrary execution of code. Another option is to escape tcl code in passed in strings. Someone will eventually forget to do this and security problems will still exist. Anyway the solution needs to ensure that arbitrary commands cannot be executed.

Please excuse my ignorance (trying to learn here),

Are not some risky tcl commands already set to null? And couldn't other ones (that are needed to be used) be renamed dynamically (on aolserver start) to some main site parameter value plus an N character randomized alphanumeric string (where N is also a parameter value (with all references to it similarly renamed)?

If tcl commands can be renamed within the context of a proc and are subsequently reset outside of its context, maybe a safe_zone proc can be made available to process user data?

Users should never be allowed to execute any code at random points like this. But if Lars' suggestion works, it is simple enough, even if ugly. There is no way to enforce users to do it that way, and I bet this is something that will keep slipping through. I don't think it is a disaster because there are probably very few users that would want to exploit the problem.

What we really need is a detailed account of the extent of the problem and how and why to correct it in one way or another. At the moment, it isn't even apparent that the problem is in ad_form, and Lars' solution indicates it isn't.

i'm also trying to understand ad_form...

how does ad_form know which data being passed is a user string not to be interpreted, developer data not to be interpreted, and developer data *to be* interpreted?

in the news instance, is it ad_form's job to know not to interpret "[open|closed]", or is should it be escaped before it ever gets there?

Well subst is being called for a purpose. The question seems to be if ad_form is being fed the correct information. ad_form cannot know, and should not care where the string came from. At least that is what I am guessing.

This is Don, not Alex ...

Greenpeace apparently has been happily doing this to do extends in loops but I think it has the same double-substitution problem that Lars's original example does, right?

for {set i 1} {$i <= $itemcount} {incr i} {

  gp_form -extend -name homepage_edit -form "
    {title_$i:text {label {Title $i}}}
    {image_$i:gp_image,multiple {label {Image $i}}}
  "

I do think the safest might be to subst the element name/type decl as well as the attr portion so we can have both dynamic names and types.

Is there any problem with this that anyone can think of?  I wasn't thinking of using ad_form for dynamic forms when I wrote it, it began as an experiment to rapidly create form handling pages when I was writing a bunch of code for Greenpeace.

But clearly being able to create truly dynamic forms is a real plus.

Check out the latest version of the parameters page for my solution (which doesn't require changes to ad_form) in action:

http://cvs.openacs.org/cvs/openacs-4/packages/acs-subsite/www/shared/parameters.tcl?rev=1.8&content-type=text/x-cvsweb-markup

I'm not so sure we should change ad_form.

/Lars

I just got back from my honeymoon, so I'm catching up on this.

I'm not quite sure what your change was, Lars. But apparently, things should work now? I'm not clear what you did to fix things.. Do we need to make any changes to our code to make it secure?

This is in 5.0, right?

Congratulations, Jade!
Congrats Jade, and then a plea...

Can someone explain this in terms us mortals might understand?

What does it really take to allow a user to break ad_form?

If for instance, I display their name in a form, is there anyway that there claiming to be "Joe [rm -rf /] Random" will get executed?

I don't really understand what Lars' changes are, but prior to the bug-fix -- yes, this is exactly what would happen: you would delete your hard disk, say, when you look at a form that lists all the users in a select list using ad_form.
Does Lars's patch have the thumbs up from those who have been using OpenACS for a while now?  Is this a patch that solves the problem completely?  Should we be installing the patch on all OpenACS sites?  Or is the jury still out?
Any comment on the new patch from Lars? This is a pretty serious issue IMO.
My solution is to not change ad_form, but make sure you use it right.

ad_form -extend ... -form [list $elm_name:$type($widget) { label $label } ... ]

May be too hard to explain properly in the docs?

It's only an issue when auto-generating forms using ad_form based on user input. The parameters issue is not critical, because only people with site-wide admin can add a package parameter.

/Lars

Hi,

I am a bit confused.  Anyway still figuring out :)  Can we include something on the ad_form api-doc about this?