Forum OpenACS Q&A: Re: Help with openacs installing

Collapse
Posted by Bjoern Kiesbye on
Hi Jim,

I think the tcl8.6 API (http://www.tcl.tk/man/tcl8.6/TclLib/SetResult.htm). should be used to manipulate the interp result, too. Even so I used the solution from nspostgres where a reference to the interp's result string is requested, using the API, and assigned to a local variable, which is then passed to sprintf instead of passing interp->result directly. I think the way it is currently done, partly defeats the purpose of the API, because the interp objects internal state is manipulated from outside code. I did use it because it was a 'known to work' solution, which i needed, and was not certain that there isn't a good reason why it is done this way.

The use of sprintf is original code from nsopenssl, I just changed the variable which is passed to it. Besides the manipulation of interp->result (assuming they will be replaced by calls to the API), sprintf is used twice more to create the channelName, and once to write to buf.

Collapse
Posted by Jim Lynch on
"Use of sprintf is original code" Doesn't make it safe. Yes, as I mentioned, I found that too, and without limiting how many bytes it writes, it has the potential of overwriting the buffer it uses. Assume, for example, that someone changes what's written by some sprintf, without knowing the size of the buffer, this has the potential to overwrite its buffer as well, especially if the sprintf arguments cause it to write more. It doesn't make for good code, it makes for the continued ignorance of an existing bug which should be fixed. I'm just trying to make sure that bugs that could cause security issues are removed and fixed, not kept, and being original code is not a reason to keep a problem.

In the case of writing integers to the tcl result, you don't have to concern yourself with either sprintf or snprintf, you can use other tcl api calls to set the result to the integer directly, and this is what is recommended.

I'll help you in any way I can to make sure these bugs no longer plague us. Would you like me to show the calls that involve writing integers to the tcl result?

Collapse
Posted by Bjoern Kiesbye on
Hello Jim,

you are right, snprintf is the preferred way, i did use that too when i was writing a simple custom gzip_http_post method. With original code I just meant, it's know to work code,and not new code introduced by the patch, which may lead to problems in case the patch is applied.
As well i was uncertain if fixing this is going to be appreciated at all, because development efforts seem to go more into NaviServer rather then Aolserver. Hearing that there is still interest i can make the changes, and would appreciated some guidance.

First Question is there a preferred way to set the interp result, as there are 2 ways Tcl_SetResult/Tcl_SetObjResult and Tcl_AppendResult. Currently the result is set to a single string, to replace it by the new API I would go with.

Example Integer:

sprintf(interp->result, "%d", nread);

is replaced by

Tcl_SetObjResult(interp, Tcl_NewIntObj(nread));




Example String(1):

interp->result = "could not register callback";

is replaced by

Tcl_SetResult(interp, "could not register callback", TCL_STATIC);



Example String(2):

sprintf(interp->result, "%s", Tcl_GetChannelName(chan));

is replaced by

Tcl_SetResult(interp, Tcl_GetChannelName(chan), TCL_VOLATILE);

Thanks Bjoern

Collapse
Posted by Jim Lynch on
Yes, -and- these API calls should work in tcl-8.5 as well.

-Jim

Collapse
Posted by Bjoern Kiesbye on
Hi Jim,

I updated the patch to use the new tcl API, at one point I had to use the Function Tcl_ObjPrintf, which is available in tcl8.5/.6 only, As the function is not available in tcl8.4 I wrapped it in a #if which checks the tcl version, and in case a the tcl version is 8.4 or lower snprintf is used instead.

Example:

+#if (TCL_MAJOR_VERSION >= 8) && (TCL_MINOR_VERSION > 4 )
+ Tcl_SetObjResult(interp, Tcl_ObjPrintf("%lu", peercert == NULL ? 0 : X509_get_version(peercert) + 1));
+#else
+ snprintf(&buf, sizeof(buf), "%lu", peercert == NULL ? 0 : X509_get_version(peercert) + 1);
+ Tcl_SetResult(interp, buf, TCL_VOLATILE);
+#endif

http://www.clever-devel.com/file/35043/nsopenssl-interp-result-0.1.1.patch

-bjoern