Disallows all-symbol names and names that are numbers < 64. Such names are difficult to kick/ban/pronounce/type. This just makes griefers' jobs a little more difficult.
Patch against 959
Allowing names with numbers >= 64 is a problem with !setlevel. Probably just not allowing any numbers or spaces at the beginning of a name is a better bet.
"//" shouldn't be allowed anywhere in a name. kicks by full name match (the vote menu in particular) will interpret // as a comment, so someone named "kevlarman@lamerspot//banme" would get me kicked instead of them unless a partial name (/kick banme) or client number was used.
Created attachment 1472[details]
Remade patch, still against 959.
Leading spaces were already stripped by SVN code (g_client.c/ClientCleanName()), so no change there.
This patch disallows names that:
Begin with a number
Contain no letters anywhere in them (are all numbers and/or symbols)
Contain "//" or "/*" anywhere in them
Created attachment 1475[details]
modified, r966
This is basically identical to attachment 1472[details]. I also fixed a bug in your patch (you used < instead of <= so only 1-8 would be counted as numbers) and make some other minor changes.
Sorry to interrupt your hard work, but this whole thing is invalid becuase you are trying to fix not the bug.
For example, if ladders can be exploited to climb higher than intended, what do you do?
1. A solution similar to this bug/post/thread would be: remove all ladder from all maps, or add physical blockers on top of all ladders. The bug is still there, but cannot be used. Stupid, stupid, stupid.
2. A perfect solution: fix the bug.
1.1.0 doesn't have any problems with kicking. It is an error of additional development, where security wan't in mind.
1. Tremulous 1.1.0 does not support searching for a name substring, but there is no problem with kicking by name. For example, to kick kevlarman, you would type /callvote kick "kevlarman@lamerspot//banme". Good, but not adequate. So:
2. Someone added code to ease kicking by searching for name substrings (that is, /callvote kick lamer would work here very well). Excellent job. Still on unpatched servers, kicking by client number has never been an issue, until:
3. Someone who doesn't know much about security consolidated /kick <name> and /clientkick <num> into /kick <name|num>. Nobody asked for such a consolidation, and I expect that anyone applying that patch should not complain (no warranty).
Removing the ability to use some names is a total pissoff for everyone's freedom. Besides, noone code can guarantee protection agains names like [&(*<!^@*&$]. Actually, the best admin menu I have ever seen is a standard UI. Right click on someone, and select kick.
More about the kick consolidation. It wasn't such a bad idea, for convenience purposes. But one kick command that "tries to predict what you want" is bad. (And I would be pissed to see it in the SVN.) Instead, I recommend something for all commands that relate to clients: generally /<cmd> <name> and /<cmd>n <num>. That is /kick <name> and /kickn <num>, !setlevel <name> and !setleveln <num>, etc.
And more about the "//" in name, it has been pointed out that using the built in command parser to do kicks and other commands is a bad way to implement votes. Use functions.
And even more about the leading spaces: nothing is wrong about them if you have a substring search kick, and require at least one alphanumeric charater.
Created attachment 1558[details]
disallow hard names, make kicking easier
This changes G_SanitiseName() so name matches ignore non-alphanumeric characters. The initial idea was to save time in banning abusive players when they use a lot of symbols in their names. In addition to being really convenient for admins, it will also make it somewhat more difficult to impersonate other users, helping prevent the wrong person from getting banned.
(In reply to comment #7)
> 2. A perfect solution: fix the bug.
Fixing the bug is not an option in this case because the bug is in the users of the software, not the software itself.
> 3. Someone who doesn't know much about security consolidated /kick <name> and
> /clientkick <num> into /kick <name|num>. Nobody asked for such a consolidation,
> and I expect that anyone applying that patch should not complain (no warranty).
This has nothing to do with security and everything to do with convenience. This prevents people from having to know the subtle difference between kick and clientkick.
> It wasn't such a bad idea, for convenience
> purposes. But one kick command that "tries to predict what you want" is bad.
DWIM. People type things without thinking too much about it and don't want to have to correct themselves. Adding code to try to figure out what they meant is much faster and saves annoyance for the user.
(In reply to comment #12)
> It is possible to use a newline as player name. Long time known bug.
Was fixed in R1122. Are you able to reproduce this on a server using a revision later than 1122?
Created attachment 2145[details]
update of the above for head
Updated the above for head. Some parts were obsolete and handled elsewhere now. Otherwise, nothing's changed. Since the original patch was written, SanitiseName has become SanitiseString, but seems to still be only used for the same set of instances that would benefit from this change. Its implementation has also since changed slightly to fix some buffer bugs. I think I have updated this correctly to leave those alone, but it'd be nice if somebody else gave it a once-over. Otherwise, this should be ready for ci, since it seemed to be ready ages ago and we just never did so.
It is still possible to pass DOS like line termination /0 into player's name and some other strings like guid to circumvent checks.
Some servers simply crashes some makes player a "ghost". Probably it depends on server's OS.
(In reply to comment #17)
> It is still possible to pass DOS like line termination /0 into player's name
> and some other strings like guid to circumvent checks.
> Some servers simply crashes some makes player a "ghost". Probably it depends on
> server's OS.
Most 1.1 servers are not updated. Those bugs were fixed here:
http://svn.icculus.org/tremulous?view=rev&revision=1193
All infostrings must pass isprint() for anyone to connect.
I just tried it on SVN head -- I hacked up a client to put a newline and a null char in my name, and was not allowed to connect because of illegal or malformed userinfo. The same is true of guid.
Compile a client using SVN head and connect to one of the protocol 70 servers (or test locally on your freshly-compiled version). If you can do it there, then reopen the bug. If not, no, you're not testing modern code.
Created attachment 1446 [details] Implementation against 959Created attachment 1472 [details] Remade patch, still against 959. Leading spaces were already stripped by SVN code (g_client.c/ClientCleanName()), so no change there. This patch disallows names that: Begin with a number Contain no letters anywhere in them (are all numbers and/or symbols) Contain "//" or "/*" anywhere in themCreated attachment 1476 [details] blah These aren't the droids you're looking forCreated attachment 2145 [details] update of the above for head Updated the above for head. Some parts were obsolete and handled elsewhere now. Otherwise, nothing's changed. Since the original patch was written, SanitiseName has become SanitiseString, but seems to still be only used for the same set of instances that would benefit from this change. Its implementation has also since changed slightly to fix some buffer bugs. I think I have updated this correctly to leave those alone, but it'd be nice if somebody else gave it a once-over. Otherwise, this should be ready for ci, since it seemed to be ready ages ago and we just never did so.