Implemented winsock info import/export #3216

weltling · 2018-04-13T09:30:50Z

This is a technique analogue to SCM_RIGHTS already available in implementations based on BSD sockets. The exported information is to be used to create a duplicate descriptor of the same network socket in another or same process. Any socket created that way can be safely closed. The actual socket is closed when all the duplicated sockets are. Note, that it only works for network sockets.

The advantage of this method over creating separate sockets using SO_REUSEADDR is, that all the duplicates have a common backlog. The API is very platform specific, thus separate functions are required.

Thanks.

weltling · 2018-04-13T09:32:09Z

@bwoebi could you check, please?

Thanks

nikic · 2018-04-13T09:36:03Z

ext/sockets/sockets.c

+			php_error_docref(NULL, E_WARNING, "Unable to export WSA protocol info [0x%08lx]: %s", err, buf);
+		}
+
+		RETURN_NULL();


Why does this return null rather than false (like the error case above)?

nikic · 2018-04-13T09:37:04Z

ext/sockets/sockets.c

+
+	RETVAL_STR((ret));
+
+	zend_string_free(ret);


Isn't this freeing the string that was just returned?

Ohh, my bad. It's just an assignment there. Fixing.

nikic · 2018-04-13T09:38:23Z

ext/sockets/sockets.c

+		php_error_docref(NULL, E_WARNING, "Faled to base64 decode protocol info");
+		RETURN_NULL();
+	}
+	memmove(&wi, ZSTR_VAL(zsinfo), ZSTR_LEN(zsinfo));


This needs to check that the string length matches the structure size beforehand.

What is the reason for using memmove over memcpy here? The structures can't overlap.

Usually memmove is safer, whereby here it's not relevant, just a habit of mine. Indeed I also miss here the length check on the supplied info len. Thanks for the comments, will tidy all that up.

nikic · 2018-04-13T09:39:51Z

ext/sockets/sockets.c

+		RETURN_NULL();
+	}
+	memmove(&wi, ZSTR_VAL(zsinfo), ZSTR_LEN(zsinfo));
+	zend_string_free(zsinfo);


As this is not performance-critical code, zend_string_release is safer.

tpunt · 2018-04-13T10:06:46Z

ext/sockets/sockets.c

+
+	zsinfo = php_base64_decode(info, info_len);
+	if (!zsinfo) {
+		php_error_docref(NULL, E_WARNING, "Faled to base64 decode protocol info");


nit: "Failed"

Fixed, thanks :)

bwoebi · 2018-04-13T10:37:20Z

ext/sockets/sockets.c

+		however it is unlikely to ever make sense to have it editable within
+		a script. The struct is suited very well and has same size under both
+		64- and 32-bit. */
+	ret = php_base64_encode((const char *)&wi, sizeof(wi));


Is there any particular need to serialize it at all? it's just simple binary data

I had it in the first variant. Basically can be done that way, but it can be weird especially when the binary data has '\0' bytes. Fe in case some output needs to be checked in tests, etc. No big deal either way otherwise.

Thanks.

I'm quite leery about directly exposing (whether encoded or not) such a structure. There's quite a lot of information in there (https://msdn.microsoft.com/en-us/library/windows/desktop/ms741675(v=vs.85).aspx) and it's not immediately obvious to me that nothing bad could happen if the user were to manually modify/construct it before calling the import function.

The concern is understandable, of course. The default security mechanism applies, when another process imports the socket. It is for sure possible to craft a WSAPROTOCOL_INFO struct.

The default security also applies, see DACLs https://msdn.microsoft.com/de-de/library/windows/desktop/aa446597(v=vs.85).aspx . Fe it is not possible use a socket created under another account, until it is explicitly set ACLs to be allowed. Or it is not possible to duplicate a socket that is not shared (didn't call WSADuplicateSocket). In general, it is hard to do harm to unrelated parts of the system, fe like "stealing" a socket from some another process, on a properly configured system. Same actually when some invalid data gets imported. In this regard, sockets using SO_REUSEADDR are a far more insecure solution.

Another approach were to use shared memory. This however introduces another implication by a need to synchronize access. AFAIR Bob was also OK to go by shared memory.

Another approach i could think of were to track the exported structure internally and introduce an extra function that encapsulates the sending process. This introduces a couple of other implications, but is in general doable.

What would you say?

Thanks.

bwoebi · 2018-04-13T10:58:14Z

Overall, I like the implementation - seems like it should also work properly in conjunction with socket_export_stream() and is flexible enough for most needs :-)

Thanks a lot for working on it!

weltling · 2018-04-13T11:21:23Z

@bwoebi great! Lets wait for more comments, otherwise I'd intend to merge this somewhen next week.

Thanks for the checks everyone!

weltling · 2018-04-23T15:16:01Z

I've now reimplemented to use shared memory mechanism. It requires an additional step for parent to wait for child's ok before releasing the memory segment, thus required an additional function. Otherwise there's no change to the suggested API, the parent will produce an identifier and send it to the child, the child will read the data structure from SHM. That way, the structure is not accessible directly in the scripts, so the import function can't be abused with a crafted data.

I've got an example echo server implementation https://gist.github.com/weltling/0d78544fa3b4c1342027229aaf7ae43c. It's a bit too big for a test, however :)

Thanks.

nikic · 2018-04-26T14:38:37Z

ext/sockets/sockets.c

+		RETURN_FALSE;
+	}
+
+	seg_name_len = snprintf(seg_name, sizeof(seg_name)-1, "php_wsa_for_%u", SOCKETS_G(wsa_child_count)++);


I think it would be better to directly zend_strpprintf to a zend_string here, as this is needed as a zend_string in two places below anyway.

nikic · 2018-04-26T14:40:03Z

Not really familiar with what this is about, someone like @bwoebi should review.

weltling · 2018-04-26T16:05:13Z

@nikic thanks for the checks, moved to strpprintf. OFC it'd be nice to hear yet from @bwoebi :) Could you please check, Bob?

In any case, this way it's ensured no untrusted data would go through the import function, though need one more synchronization step to not to waste the spent memory. Actually could make both ways work, but i think the more secure way like this one should be preferred.

Thanks.

weltling · 2018-05-04T09:27:33Z

@bwoebi ping :)

bwoebi · 2018-05-15T13:28:42Z

Looks good to me generally - I'd appreciate a merge :-)

weltling · 2018-05-19T13:40:06Z

Merged as 9d4da80. Thanks everyone!

nikic reviewed Apr 13, 2018

View reviewed changes

tpunt reviewed Apr 13, 2018

View reviewed changes

bwoebi reviewed Apr 13, 2018

View reviewed changes

Implemented winsock info import/export

f126702

weltling force-pushed the wsaprotocol_info branch from d34f8d0 to f126702 Compare April 23, 2018 15:03

Missed to readd the test

585ea1a

nikic reviewed Apr 26, 2018

View reviewed changes

nikic added the Waiting on Review label Apr 26, 2018

Switch to strpprintf

4d0066a

weltling closed this May 19, 2018

weltling deleted the wsaprotocol_info branch May 29, 2018 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented winsock info import/export #3216

Implemented winsock info import/export #3216

weltling commented Apr 13, 2018

weltling commented Apr 13, 2018

nikic Apr 13, 2018

nikic Apr 13, 2018

weltling Apr 13, 2018

nikic Apr 13, 2018

weltling Apr 13, 2018

nikic Apr 13, 2018

tpunt Apr 13, 2018

weltling Apr 13, 2018 •

edited

Loading

bwoebi Apr 13, 2018

weltling Apr 13, 2018

nikic Apr 13, 2018

weltling Apr 13, 2018

bwoebi commented Apr 13, 2018

weltling commented Apr 13, 2018

weltling commented Apr 23, 2018

nikic Apr 26, 2018

nikic commented Apr 26, 2018

weltling commented Apr 26, 2018

weltling commented May 4, 2018

bwoebi commented May 15, 2018

weltling commented May 19, 2018

Implemented winsock info import/export #3216

Implemented winsock info import/export #3216

Conversation

weltling commented Apr 13, 2018

weltling commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltling Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwoebi commented Apr 13, 2018

weltling commented Apr 13, 2018

weltling commented Apr 23, 2018

Choose a reason for hiding this comment

nikic commented Apr 26, 2018

weltling commented Apr 26, 2018

weltling commented May 4, 2018

bwoebi commented May 15, 2018

weltling commented May 19, 2018

weltling Apr 13, 2018 •

edited

Loading