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

Collapse
46: Re: Help with openacs installing (response to 45)
Posted by Jim Lynch on 07/02/14 04:50 PM
Hi...

I would like to tell about my exp getting 8.6tcl to work with some of the aolserver modules...

Before 8.6, the tcl result was available as a string, with some funcs to set and append to that string.

At 8.6, you actually have a choice, you can rebuild it so it works as before (so result is exposed) or you can alter code which accesses the result, as the patch suggests.

What I found out was, if you want numeric result values, you can have tcl set them as ints or maybe longs (or other object types). This is what I preferred to do when I was trying to get 8.6 going.

You want to be very cautious with sprintf, as this func doesn't limit the writing based on length. So, you either want to use things that append to the string result object, or you want to set as cast to a different type, for example int. 8.6 provides calls for doing either.

Almost every time I've seen uses of sprintf (in -anything-), I knew there was the typical "buffer overrun" possibility, and in the cases where I could, I was able to replace each with calls to snprintf (or to use a different alternative that was also safe).

As an aside, many years ago, when looking at aolserver source for my first time, I happened to look at the call Ns_DStringPrintf, which then used sprintf. I wrote an equivalent using snprintf, and aolserver devs eventually took a whole printf func from some stdio lib source, and adapted it so that it "printf"ed to a DString in a safe manner. To Gustaf, if you hadn't done so already, would you be willing to look at it and at naviserv source to see if it's been incorporated already?

If you're going to use sprintf, get the length of the buffer and use snprintf instead, as not doing so could result in a crash if sprintf overwrites a dynamically allocated buffer.

This may sound like hyperbole, but it's my very stong belief that sprintf is never safe, and should always be replaced with snprintf.

I may still have the sources of other aolserver or naviserv (don't remember which at this moment) modules, where I had to rewrite the pieces that set or get the tcl result.

On request, I'll put something together in the hope it will assist.

-Jim

Collapse
47: Re: Help with openacs installing (response to 46)
Posted by Bjoern Kiesbye on 07/02/14 06:38 PM
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
50: Re: Help with openacs installing (response to 47)
Posted by Jim Lynch on 07/04/14 01:04 AM
"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
51: Re: Help with openacs installing (response to 50)
Posted by Bjoern Kiesbye on 07/07/14 06:58 PM
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
52: Re: Help with openacs installing (response to 51)
Posted by Jim Lynch on 07/07/14 10:25 PM
Yes, -and- these API calls should work in tcl-8.5 as well.

-Jim

Collapse
53: Re: Help with openacs installing (response to 52)
Posted by Bjoern Kiesbye on 07/09/14 12:02 AM
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