Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#8384 closed defect (wontfix)

excludeStart syntax in build slight security risk

Reported by: dante Owned by: James Burke
Priority: high Milestone: tbd
Component: BuildSystem Version: 1.2.3
Keywords: Cc:
Blocked By: Blocking:

Description

It came up in conversation about documenting the excludeStart syntax and what variables are available for comparing that because the build system runs in rhino and has escalated access to the filesystem.

buildUtil.js does:

isTrue = !!eval("(" + condition + ")");

to determine if a exclude block is applicable. If the build were running over untrusted code (say, a hosted build service) this could lead to a lot of problems.

worth noting in the documentation, not sure anything can be done about this in code. we eval a number of other places in build, but none seem as risky as evaluating code found in comments.

Change History (3)

comment:1 Changed 10 years ago by James Burke

Hmm, the comments have to follow a certain syntax, so I it is not like any comment gets evaled. Also, given that we do eval in other places, I think the major issue is just trusting the JS files on your disk.

An easier exploit seems to just change some JS module to do a bunch of Java calls to do the work, that way at least you get line breaks and vars. :)

But it seems to me the bigger issue is having untrusted files in your build area. I am not sure commenting specifically about excludeStart is worth it -- it may be useful to say the build system in general should only be used with trusted code though.

comment:2 Changed 10 years ago by dante

Resolution: wontfix
Status: newclosed

from what i see glancing through our other eval usage is based on built-up strings (like matching require()'s etc) ... but yes, the build should only be run on trusted code or jailed in some manner, and it is just worth noting, especially when we think about introducing the webbuild again.

comment:3 Changed 10 years ago by James Burke

dante: yes, you are right, it is not as bad a full file eval. You could still sneak some things in like a dojo.requireIf(), looks like a similar exposure to the exludeStart stuff.

Note: See TracTickets for help on using tickets.