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

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.

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

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

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.

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.

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.

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 am a bit confused.  Anyway still figuring out :)  Can we include something on the ad_form api-doc about this?