SIP 93/94 behavior non-spec, illogical

[Joe Atzberger]

The wiki page here says the 93 login is required as the first message:
   http://evergreen-ils.org/dokuwiki/doku.php?id=evergreen-admin:sip_support#login

If true, that is bad, because 93 is only defined for SIP2. That would prevent EG from supporting SIP v1 clients.

The 93/94 behavior is already non-spec, however. For example, the response to invalid terminal username or password is to drop the connection outright. That doesn't make sense, since we should at least return a 940 first. The client would never get a positive indication of login failure, and so might predictably attempt to reconnect and login with the same info again, forever.

Using a valid terminal username/password, but an invalid location code returns a "successful" 941, then drops the connection:
===
telnet localhost 8023
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
login: sip_01
password: sip_01
Login OK. Initiating SIP
9300CNsip_01|COsip_01|CPZZZ|
941
Connection closed by foreign host.
===

So that's even worse, because we tell the client the login was "completely successful" and then drop them. They might be expected to reconnect using the same login string again, forever.

[Joe Atzberger (ohiocore)]

This behavior appears to be different between the "RAW" connection method vs. telnet. The RAW behavior is more appropriate (and the 93 requirement fine there).

$ telnet localhost 6001
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
9300CNsip_01|COsixxx|CPZZZ|
940
9300CNsip_01|COsixxx|CPZZZ|
940
9300CNsip_01|COsixxx|CPZZZ|
Connection closed by foreign host.

[Dan Scott (denials)]

I'll admit that I don't understand why this has been classified as a security vulnerability.

[Dan Scott (denials)]

Joe, did you file this bug as a security vulnerability because of the possibility of a reverse DoS of the clients? I don't see any information leaks or much potential for exploits here. It just seems like a spec compliance bug (or several of them) that should be filed openly. If I'm wrong, please enlighten me! The silence of the rest of the people on the security team means that it's either blindingly obvious to them, or they're in the dark like me, or they're too busy to pay attention if the implications aren't clear.

[Jason Etheridge (phasefx)]

I don't get the security implications either.

[Joe Atzberger (ohiocore)]

You have both a false positive for the 9300 login, and the potential to DOS your SIP by churning through repeated attempts.

Feel free to refile this as non-security if you think it is clearer or more helpful that way. It wasn't my intent to post it in secret.

[Jason Stephenson (jstephenson)]

Looks like we don't support SIP v1 clients. (I've never seen such a thing, personally.)

Also looks like we may have an issue with how 93/94 message pairs are handled.

I'll take a look to see what I can determine from the code. This may also affect SIPServer and not just Evergreen.

Or, am I way off base and was my first instinct that this is an invalid or opinion bug, correct?

[Joe Atzberger (ohiocore)]

I don't think this can be classified opinion. The spec is the spec, and this behavior is on the wrong side of it.

Also, you have two different behaviors for the same input based on which "channel" is used. RAW tells you you failed, telnet tells you you succeeded, then fails.

In practice the EG userbase is on SIP2 via RAW. They don't see these problems, so importance is low. But it's still a bug.

[Jason Stephenson (jstephenson)]

Looks like a SIPServer bug and not an Evergreen bug. Evergreen does the same thing regardless of the SIP authentication method.

[Jason Stephenson (jstephenson)]

So, looks like 2 issues here:

1. We should not require the 93 over telnet and use the login and password responses to log the client in to the backend.

2. If we get an invalid 93 over telnet, then we should respond properly with a 940 and not just drop the connection to the client.

On 1, I suppose SIPServer could generate the 93 on its own and send it to the backend.

On 2, looks like sip_protocol loop should be modified to not require the SC_STATUS as the first message.