Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EXC_BAD_ACCESS if ICE server config contains usernames with %40 instead of @ #157

Closed
derwaldgeist opened this issue Apr 12, 2016 · 12 comments

Comments

@derwaldgeist
Copy link
Contributor

There are free TURN servers hosted by Viagenie:
http://numb.viagenie.ca/

If you register for an account, your credentials will look like that:

{
        "urls": "turn:numb.viagenie.ca",
        "credential": "<yourkey>",
        "username": "user%40domain.com"
}

The %40 causes the iosrtc plugin to break with EXC_BAD_ACCESS at this line:
https://github.com/eface2face/cordova-plugin-iosrtc/blob/master/src/PluginRTCPeerConnectionConfig.swift#L23

NSLog requires % signs to be escaped with %%, since they have a special meaning:
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html#//apple_ref/doc/uid/TP40004265

If I change the %40 to @, it works. But %40 is valid and used by providers like Viagenie, so I think it should be supported by the plugin.

It can be fixed like this:

let escapedUrl = url!.stringByReplacingOccurrencesOfString("%", withString: "%%")
let escapedUsername = username.stringByReplacingOccurrencesOfString("%", withString: "%%")
let escapedPassword = password.stringByReplacingOccurrencesOfString("%", withString: "%%")
NSLog("PluginRTCPeerConnectionConfig#init() | adding ICE server [url:\(escapedUrl), username:\(escapedUsername), password:\(escapedPassword)]")

I can create a PR for this if you want to.

@ibc
Copy link
Collaborator

ibc commented Apr 12, 2016

Good catch. However I expect this to happen in much more cases as the plugin is logging user entered input. I mean: if the user calls pc.setLocalDescription(desc) and desc.sdp contains a % symbol it would also crash (because the plugin also prints the given SDP).

So AIFAU this must be sanitized in the Swift code rather than in JS land. And this should be done by adding a new Swift module that internally calls NSLog() with the sanitized text message.

Does it make sense? A PR handling this would be really welcome :)

@ibc
Copy link
Collaborator

ibc commented Apr 12, 2016

Or perhaps I should refactor how input is given to NSLog and instead of:

NSLog("PluginRTCPeerConnectionConfig#init() | adding ICE server [url:\(url!), username:\(username), password:\(password)]")

use this:

NSLog("PluginRTCPeerConnectionConfig#init() | adding ICE server [url:%s, username:%s, password:%s]", url!, username, password)

?

@derwaldgeist
Copy link
Contributor Author

I'm not a Swift expert (learned it a while ago and haven't use it often since), so I'm not sure if switching the syntax to %s will help, but I would give it a shot (have you tried this already?). Easier than replacing all NSLogs() by a custom module. But this would be the next option.

@ibc
Copy link
Collaborator

ibc commented Apr 13, 2016

Unfortunately I won't be able to try it for a while...

@derwaldgeist
Copy link
Contributor Author

OK, I will check if it is possible and give feedback. Currently, I am using my own fork anyway, since I am waiting for your feedback on PR #154.

@ibc
Copy link
Collaborator

ibc commented Apr 14, 2016

@derwaldgeist I've got some time today to work on the plugin and I'm handling other pending issues/PRs. If you could test whether using my above suggestion (at least in the crashing line) fixes the problem, please comment it and I'll fix all the other calls to NSLog().

@derwaldgeist
Copy link
Contributor Author

@ibc: I just tested it the minute you posted your comment :-) And yes, the %s fix works!

@ibc
Copy link
Collaborator

ibc commented Apr 14, 2016

Great! I will fix all the other cases.

@derwaldgeist
Copy link
Contributor Author

Oops, accidentally closed the issue.

@derwaldgeist
Copy link
Contributor Author

Perfect!

@ibc
Copy link
Collaborator

ibc commented Apr 14, 2016

One more thing, may you please check what happens if password is null or has not been set? I hope it doesn't crash...

NSLog("PluginRTCPeerConnectionConfig#init() | adding ICE server [url:%s, username:%s, password:%s]", url!, username, password)

@ibc
Copy link
Collaborator

ibc commented Apr 14, 2016

I think I've got the solution:

let a = 123.123
var b: [String] = ["Eggs", "Milk"]
var c: Int!
var d = "user%40domain.com"

NSLog("a: '%@'", String(a))
NSLog("b: '%@'", String(b))
NSLog("c: '%@'", String(c))
NSLog("d: '%@'", String(d))
NSLog("d: \(d)")

prints:

2016-04-14 04:39:18.726 swift[23733:21181822] a: '123.123'
2016-04-14 04:39:18.727 swift[23733:21181822] b: '["Eggs", "Milk"]'
2016-04-14 04:39:18.728 swift[23733:21181822] c: 'nil'
2016-04-14 04:39:18.728 swift[23733:21181822] d: 'user%40domain.com'
2016-04-14 04:39:18.728 swift[23733:21181822] d: user                                       0omain.com  // Obviously wrong

@ibc ibc closed this as completed in 2cb978e Apr 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants