Jump to content
Eternal Lands Official Forums
Sign in to follow this  
KarenRei

strncpy and the like

Recommended Posts

Earlier I noted that there was a lot of use of strcpy and similar functions: functions which write to a string without having the allocated space for that string specified. Such functions are at risk of causing crashes and pose security risks. Most people here seemed aware of this.

 

However, in my fixes of these functions, I've found an error in the "fix" used by a number of well-meaning people. I've run into quite a bit of code to the effect of:

 

blah[512];

void do_something(char* str)
{
  strncpy(blah, str, sizeof(blah));
}

 

This is likely an error. The basic principles are there: using strncpy to specify the length of the buffer you're writing into, using sizeof instead of a fixed integer to get the length, etc. However, from the man page:

 

The strncpy() function is similar, except that not more  than  n
bytes  of  src  are copied. Thus, if there is no null byte among
the first n bytes of src, the result  will  not  be  null-terminated.

 

So, let's say that you did try and pass a string that was too long to do_something. This function won't crash, sure. However, blah is now a non-null-terminated string. Try to do almost any sort of string operation on it -- print it, use it as the source in a strcpy-type function, etc -- and it'll probably blow up your code. In short, this is better than strcpy, but not by much.

 

You need to leave room for the null, and then pad it:

 

blah[512];

void do_something(char* str)
{
  strncpy(blah, str, sizeof(blah) - 1);
  blah[sizeof(blah) - 1] = '\0';
}

Share this post


Link to post
Share on other sites

Well, this are low level security bugs, and will not likely affect the game or the security of the people that play on our official server and don't use any unofficial proxies.

Not sure if they should be fixed right now, but if you can reliably fix them, without introducing new bugs or new chunks of code, I guess it would be OK to do it while we are in the feature freeze.

Share this post


Link to post
Share on other sites

They're pretty simple fixes, so I'll go through and take care of them. I only mentioned it so that people know for future coding. Right now, each of these is only a "potential" exploit, and a "potential" crash. Most of them are probably safe.

Share this post


Link to post
Share on other sites

okay, I have one :P

blah[512];

void do_something(char* str)
{
  strncpy(blah, str, sizeof(blah) - 1);
  blah[sizeof(blah) - 1] = '\0';
}

the problem is, you have to make sure you have room for an extra char before you send in, because this potentially chops the last char off (in other places, where there's a real strlen sent in and the last char is always used as a null, it's going to be wron 100%, whether that's needed for safety or not)... I'd fix it, but I'm not sure if fixing just the functions themself is good (blah[sizeof(blah)] = '\0'), or if some of the places where they're called need checks as well

 

for an example, right now all the messages from server that are shown on the screen are missing the last character (eg the trailing ']' in PMs, etc)

Edited by ttlanhil

Share this post


Link to post
Share on other sites

Algorithm: That posted algorithm will only truncate the last character if your string is so large that it would fill up the entire buffer without a null. If you full up the entire buffer without a null at the end, what you have is no longer a c string, and any subsequent use of C string functions on it will have undefined, risky behavior. It is much better to truncate the last character, if it would fill up the target buffer, than to leave it in place.

 

You really have two cases that need to be dealt with.

 

Case 1) Replacing strcpy. In this case, you only need one size argument -- the sizeof the destination buffer. The function that I created for this is safe_strncpy(dest, src, len).

 

Case 2) Replacing strncpy in the specific case where you only want a certain number of characters transferred. In this case, you need two size arguments. The function that I created for this is safe_strncpy2(dest, src, dest_len, src_len).

 

Server truncation: Just noticed that myself. Fixing. This is actually a different issue, and not one that applies to the general case. The general case fix that I used was a function that takes a buffer length plus how many characters you want to copy, not a single max size. This particular bug was just an oversight on my part which only affected raw text from the server. Raw text used a memcpy, not a strcpy, so I had to special case it.

Edited by KarenRei

Share this post


Link to post
Share on other sites

Just caught another in chat: I used sizeof(somevar) when the variable was a char*, not a char[] like I mistakenly thought it was. Fixed.

Edited by KarenRei

Share this post


Link to post
Share on other sites

Sigh ... I had meant to fix them when you find them, not to go thru and do a massive patch for possible bugs. Now everyone of those changes needs to be tested for new bugs!

Share this post


Link to post
Share on other sites

I asked if it would be okay to do. I was told that it would be, if they were low-risk fixes only (they were). There were many of them, but they were all pretty straightforward, and each one of them fixed a potential way our users' clients could be attacked (a few of them fixed ways that I know for a fact could have been cracked, and this was without doing a detailed investigation, looking for a way in). So, I fixed them. Due to the number of fixes, of course a few slipped through the cracks. However, they were pretty straightforward changes; I don't expect many, if any, more bugs to be encountered.

 

Please don't complain retroactively after I was given permission. There's nothing worse than being told to do something and then being told that you were wrong to do it.

Edited by KarenRei

Share this post


Link to post
Share on other sites

I asked if it would be okay to do. I was told that it would be, if they were low-risk fixes only (they were). There were many of them, but they were all pretty straightforward. So, I did them. Due to the number of them, of course a few slipped through the cracks. However, they were pretty straightforward changes; I don't expect many, if any, more bugs to be encountered.

 

Please don't complain retroactively after I was given permission. There's nothing worse than being told to do something and then being told that you were wrong to do it.

First of all I was statingwhen I said to do it, what I ment. Also, Entropy, had said he wasn't sure if they should be or not. You've already fixed some bug and my compiler complained about one in actors.c line 925 and another in chat.c line 1616. Thats enough to show that all the changes need to be checked to make sure no other hidden bugs are there, like using the wrong length or other off by one errors.

Share this post


Link to post
Share on other sites

I was referring to Entropy. He said that if I could reliably fix them without introducing new chunks of code, to do so. I felt that I could, so I did so. If you ever don't want me to do something, please tell me beforehand.

 

I was just about to put in the fix for your warnings, but it looks like you already did it. Part of the problem is that I get so many warnings, it's easy t have new ones get lost in the clutter. Perhaps, after feature freeze ends, I could clean them up. I put "after feature freeze" in there because I don't want to get complained about again. It may be wise to compile with -Werror, then, wouldn't you think?

Edited by KarenRei

Share this post


Link to post
Share on other sites

I was referring to Entropy. He said that if I could reliably fix them without introducing new chunks of code, to do so. I felt that I could, so I did so. If you ever don't want me to do something, please tell me beforehand.

 

I was just about to put in the fix for your warnings, but it looks like you already did it. Part of the problem is that I get so many warnings, it's easy t have new ones get lost in the clutter. Perhaps, after feature freeze ends, I could clean them up. I put "after feature freeze" in there because I don't want to get complained about again. It may be wise to compile with -Werror, then, wouldn't you think?

Actually Lachesis checked those in.

 

As for all those warnings, when I've been in a portion of the code and see warnings I try to reduce the number of warnings, specially when it's a simple fix, others have been doing this as well. I've often done compiles without a make clean when I've changed files just so I can see warnings in the files I just changed to make sure I haven't added bugs, and to see if there are obvious warnings that need fixing.

Share this post


Link to post
Share on other sites

Well, for an example of warnings getting lost, I'd present the example of chat.c:

 

chat.c: In function ‘close_channel’: chat.c:244: warning: pointer targets in passing argument 2 of ‘my_tcp_send’ differ in signedness
chat.c: In function ‘change_to_current_chat_tab’: chat.c:536: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:536: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:536: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:536: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:540: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:540: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:540: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:540: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:544: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:544: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:544: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:544: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c: In function ‘root_key_to_input_field’:
chat.c:756: warning: pointer targets in passing argument 1 of ‘check_var’ differ in signedness
chat.c:758: warning: pointer targets in passing argument 1 of ‘send_input_text_line’ differ in signedness
chat.c:763: warning: pointer targets in passing argument 1 of ‘test_for_console_command’ differ in signedness
chat.c:769: warning: pointer targets in passing argument 1 of ‘send_input_text_line’ differ in signedness
chat.c:771: warning: pointer targets in passing argument 1 of ‘add_line_to_history’ differ in signedness
chat.c:824: warning: pointer targets in passing argument 2 of ‘put_string_in_buffer’ differ in signedness
chat.c: In function ‘put_string_in_input_field’:
chat.c:894: warning: pointer targets in passing argument 1 of ‘safe_snprintf’ differ in signedness
chat.c: In function ‘create_chat_window’:
chat.c:936: warning: pointer targets in passing argument 1 of ‘create_window’ differ in signedness
chat.c: In function ‘init_channel_names’:
chat.c:1147: warning: pointer targets in passing argument 2 of ‘xmlGetProp’ differ in signedness
chat.c:1147: warning: pointer targets in assignment differ in signedness
chat.c:1161: warning: pointer targets in passing argument 1 of ‘xmlStrdup’ differ in signedness
chat.c:1161: warning: pointer targets in assignment differ in signedness
chat.c:1165: warning: pointer targets in passing argument 2 of ‘xmlGetProp’ differ in signedness
chat.c:1165: warning: pointer targets in assignment differ in signedness
chat.c:1185: warning: pointer targets in passing argument 1 of ‘strlen’ differ in signedness
chat.c:1190: warning: pointer targets in assignment differ in signedness
chat.c:1194: warning: pointer targets in passing argument 1 of ‘xmlStrdup’ differ in signedness
chat.c:1194: warning: pointer targets in assignment differ in signedness
chat.c:1200: warning: pointer targets in passing argument 2 of ‘xmlGetProp’ differ in signedness
chat.c:1200: warning: pointer targets in assignment differ in signedness
chat.c:1216: warning: pointer targets in passing argument 2 of ‘xmlGetProp’ differ in signedness
chat.c:1216: warning: pointer targets in assignment differ in signedness
chat.c:1230: warning: pointer targets in passing argument 1 of ‘xmlStrdup’ differ in signedness
chat.c:1230: warning: pointer targets in assignment differ in signedness
chat.c:1238: warning: pointer targets in passing argument 1 of ‘strlen’ differ in signedness
chat.c:1243: warning: pointer targets in assignment differ in signedness
chat.c:1247: warning: pointer targets in passing argument 1 of ‘xmlStrdup’ differ in signedness
chat.c:1247: warning: pointer targets in assignment differ in signedness
chat.c:1252: warning: pointer targets in passing argument 1 of ‘strlen’ differ in signedness
chat.c: In function ‘tab_bar_button_click’:
chat.c:1415: warning: pointer targets in passing argument 2 of ‘my_tcp_send’ differ in signedness
chat.c: In function ‘chan_int_from_name’:
chat.c:1507: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:1507: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c: In function ‘display_chan_sel_handler’:
chat.c:1567: warning: pointer targets in passing argument 3 of ‘draw_string_zoomed’ differ in signedness
chat.c:1587: warning: pointer targets in passing argument 3 of ‘draw_string_zoomed’ differ in signedness
chat.c: In function ‘tab_special_click’:
chat.c:1637: warning: pointer targets in passing argument 1 of ‘create_window’ differ in signedness
chat.c: In function ‘create_tab_bar’:
chat.c:1805: warning: pointer targets in passing argument 1 of ‘create_window’ differ in signedness
chat.c: In function ‘change_to_current_tab’:
chat.c:1838: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:1838: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:1838: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:1838: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:1842: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:1842: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:1842: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:1842: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:1846: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:1846: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c:1846: warning: pointer targets in passing argument 1 of ‘my_strncompare’ differ in signedness
chat.c:1846: warning: pointer targets in passing argument 2 of ‘my_strncompare’ differ in signedness
chat.c: In function ‘chan_target_name’:
chat.c:2020: warning: pointer targets in passing argument 2 of ‘chan_int_from_name’ differ in signedness

 

In output like that, it can be hard to even see new errors, much less new warnings. Which is why, after feature freeze, I'd like to clean it up, if that's okay with you.

Edited by KarenRei

Share this post


Link to post
Share on other sites

Anyway, it seems that the last changes (I assume your changes, not entirely sure) caused a lot of problems, like lines of chat not being properly displayed and stuff like that (see the bugs forum, some are pretty recent).

I was assuming they would be small fixes, but if they cause bugs they should be postponed. The security risk is minimal (only if someone hacks our server and is able to send malicious packets to the clients, which is pretty unlikely), so it's better not to touch that code if it causes problems we didn't have before.

Share this post


Link to post
Share on other sites

These were not risks for someone hacking your server. I haven't done any checks for the safety of your server; since I can't see the code, I'm just assuming that it's good. These were all risks for things either originating from other clients or from man-in-the-middle or blind man attacks. I already showed you one example of an "originating from other clients" exploit and one example of a man-in-the-middle/blind man exploit, both of which were patched up here. However, these were not the only ones -- far from it. Also hooking them in revealed that the server is sending undersized GET_ACTIVE_SPELL packets (or at least reporting that they're too small).

 

Concerning bugs in the fixes: there was one cosmetic bug reported, and one cosmetic bug that I discovered on my own. Haven't heard of any since. Of course, it's your call if you want to rollback the patch. It's just, leaving open security holes kind of bothers me. If it's a choice between security holes or an occasional bug that makes the occasional string come up a character too short (which I'd patch as soon as it came to my attention), I know which I'd choose.

Edited by KarenRei

Share this post


Link to post
Share on other sites

The reported bugs were quite severe, and it seem they happened way too often. Like I said before, if you have a man in the middle attack, then you will have much more serious security problms than just with EL.

The other attack you pointed out was valid though, and it should be fixed (although at least one person had their whole X crash following your fix).

Share this post


Link to post
Share on other sites

Which severe bugs are you referring to? The only severe bug was the fix to the URL exploit. The original fix had a bug in it; Lachesis attempted to fix the bug, but in a way, it ended up worse (will be fixed tonight). All of the ones pertaining to strings were just cosmetic truncations that are trivial to fix (I'm running a careful audit of every change right now, and it's turning up precious few bugs -- results of that this evening). Also, I'm not sure you realize how easy Man in the Middle attacks are if you're on the same network. Try out "Hunt", for example: http://www.securiteam.com/tools/3X5QFQUNFG.html. Or PATH: http://p-a-t-h.sourceforge.net/html/index.php. Or any of these: http://www.securityforest.com/wiki/index.p...sion_Hi-Jacking.

 

As for the risk, I'm not talking about compomised EL passwords, but compromised machines. To me, it's never been about the security of people's EL accounts, because that's a matter that would have to be addressed after update; it's too big for mere bugfixes**. I'm far more concerned about the fact that people assume they're using software that won't let their systems get hacked. Lastly, as I've mentioned before, Man-In-The-Middle attacks are not the only type you're at risk for. My biggest concerns are from attacks of the browser URL-type. To me, the nightmare scenario is a user who can walk around in EL and knock the systems of everyone they see at will. And there's a lot of potential for that when almost the entire program was doing risky string operations. I wasn't specifically trying to come up with such specific exploits for them, but I have little doubt that I could have if I wanted to. As could anyone else who's ever done this sort of thing before. So far, the small size of EL has saved you from these sorts of things (like the reason why there are so many more PC viruses than Mac viruses), but you're still taking a gamble, each day that goes by.

 

But, as I said, your call. :D I know you have a schedule to think about. Still, given that we can't release until these segfaults get fixed and given that I'm fixing my bugs far faster than any progress is being made on them, what's the harm?

 

Anyways, I'll respect any decision that you make. If you want me to roll it back, just say the word. I can try to do so, then reapply all of the changes that have been made since then, then redo the browser/URL fix and the other criticals. Just let me know.

 

** An example of why I'm not even bothering with holes that would let a player's account get compromised is right here:

 

len= strlen(password_str);
 for(j=0; j<len; j++) str[i+j+1]= password_str[j];
 str[i+j+1]= 0;

 len= strlen(str);
 len++;//send the last 0 too
	if(my_tcp_send(my_socket, str, len)<len)
			{
					//we got a nasty error, log it
			}
	my_tcp_flush(my_socket);	// make sure tcp output buffer is empty

 

Plaintext password authentication -- not even MD5'ed with salt sent from the server. When that's the starting point, there's nothing I can do. Thus, I've only been focused on things that would allow a *victim's computer* to be compromised.

Edited by KarenRei

Share this post


Link to post
Share on other sites

Hint: use

CWARN=-Wall -Wno-sign-compare -Wno-pointer-sign

CXXWARN=-Wall -Wno-sign-compare

in your make.conf in order to get all warnings but sign problems.

Share this post


Link to post
Share on other sites

The URL exploit was the only one that could have caused some trouble, but it wasn't that bad, because it required the user to actually press F2 and it was platform specific. Now, of course I am not saying it was good we had that, but it wasn't the end of the world either.

To me, the nightmare scenario is a user who can walk around in EL and knock the systems of everyone they see at will.

It can never happen, the server is very strict with the data it passes to the clients.

FYI, the server code is written mostly by me, and with some code from a few other people (Learner for example). I am a very security aware person when it comes to the server, which explains the small number of exploits found over the years. And each time we found an exploit, we usually fixed it within hours.

 

The client has code from a few dozen of people, many of them who are not very security aware, or don't understand the whole code, so it can conflict with other's code.

Share this post


Link to post
Share on other sites

Hint: use

CWARN=-Wall -Wno-sign-compare -Wno-pointer-sign

CXXWARN=-Wall -Wno-sign-compare

in your make.conf in order to get all warnings but sign problems.

I've just been using -Wall -W, and ignoring most of the signed issues except when I want to see if some cane be cleaned up easily.

Share this post


Link to post
Share on other sites

Just a quick question for after release: would there be interest in making the default build use -Werror, forcing the cleanup of these warnings and preventing the introduction of potentially problematic code in the future?

Share this post


Link to post
Share on other sites

Just a quick question for after release: would there be interest in making the default build use -Werror, forcing the cleanup of these warnings and preventing the introduction of potentially problematic code in the future?

I suggest don't make the default build -Werror until -Werror compiles properly. That might cause a huge number of posts in the forums from people using the CVS setup and not understanding how compiling works or how to change it.

Share this post


Link to post
Share on other sites

Oh, certainly; that's what I meant. I had eariler offered to go through at get rid of at least all of the warnings that show up in gcc/g++ after release. Wouldn't want to do it before release because, as we saw with strncpy, when there are that many fixes that need to be make, even an error rate of a little over 1% can cause problems for others. And most of these would be cosmetic/good coding practices, not things as important as security.

 

It'd still probably break for users of other compilers, but there's nothing I could do about that; they'd need to either clean up the warnings themselves or report them on the forum for me to clean up.

Edited by KarenRei

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
Sign in to follow this  

×