The ws2_32.dll is needed by JingleThreadWrapper for WebRTC and Chromoting.

The ws2_32.dll is needed by JingleThreadWrapper for WebRTC and Chromoting.
We can't avoid this dependency (WSACreateEvent) in a short time, so load ws2_32.dll again as a short term solution.

BUG=115501
TEST=Build and run webrtc test on Windows.

[Sergey Ulanov, 2012-02-24 20:26:43]

LGTM

http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_p...
File chrome/renderer/chrome_render_process_observer.cc (right):

http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_p...
chrome/renderer/chrome_render_process_observer.cc:225: // ws2_32.dll is used by
JingleThreadWrapper for WebRTC and Chromoting.
nit: add an empty line above this comment.

http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_p...
chrome/renderer/chrome_render_process_observer.cc:226: // TODO(ronghuawu):
Remove this once we can get rid of the WSACreateEvent.
May be better to explain that this is due to SocketServer dependency

[jschuh, 2012-02-24 20:38:28]

Sorry for the drive-by, but Winsock is absolutely forbidden in the renderer
process. If you initialize it a compromised renderer will have full access to
the network device, which is a security regression we cannot allow.


On 2012/02/24 20:26:43, sergeyu wrote:
> LGTM
> 
>
http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_p...
> File chrome/renderer/chrome_render_process_observer.cc (right):
> 
>
http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_p...
> chrome/renderer/chrome_render_process_observer.cc:225: // ws2_32.dll is used
by
> JingleThreadWrapper for WebRTC and Chromoting.
> nit: add an empty line above this comment.
> 
>
http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_p...
> chrome/renderer/chrome_render_process_observer.cc:226: // TODO(ronghuawu):
> Remove this once we can get rid of the WSACreateEvent.
> May be better to explain that this is due to SocketServer dependency

[cpu_(ooo_6.6-7.5), 2012-02-24 20:39:55]

You don't plan on opening tcp/udp sockets from a renderer, right?

[Sergey Ulanov, 2012-02-24 20:44:53]

On Fri, Feb 24, 2012 at 12:38 PM, <jschuh@chromium.org> wrote:

> Sorry for the drive-by, but Winsock is absolutely forbidden in the renderer
> process. If you initialize it a compromised renderer will have full access
> to
> the network device, which is a security regression we cannot allow.


We don't really use winsock in the renderer (not creating any sockets),
it's just that some code in libjingle depends on winsock. It worked fine
previously, but stopped after winsock dependency was dropped from ffmpeg
dlls. We'll need to fix libjingle to not depend on winsock APIs, but until
that's done we need this fix to keep webrtc and remoting working.


>
>
>
> On 2012/02/24 20:26:43, sergeyu wrote:
>
>> LGTM
>>
>
>
> http://codereview.chromium.**org/9463023/diff/1/chrome/**
>
renderer/chrome_render_**process_observer.cc<http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_process_observer.cc>
>
>> File chrome/renderer/chrome_render_**process_observer.cc (right):
>>
>
>
> http://codereview.chromium.**org/9463023/diff/1/chrome/**
>
renderer/chrome_render_**process_observer.cc#newcode225<http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_process_observer.cc#newcode225>
>
>> chrome/renderer/chrome_render_**process_observer.cc:225: // ws2_32.dll
>> is used
>>
> by
>
>> JingleThreadWrapper for WebRTC and Chromoting.
>> nit: add an empty line above this comment.
>>
>
>
> http://codereview.chromium.**org/9463023/diff/1/chrome/**
>
renderer/chrome_render_**process_observer.cc#newcode226<http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_process_observer.cc#newcode226>
>
>> chrome/renderer/chrome_render_**process_observer.cc:226: //
>> TODO(ronghuawu):
>> Remove this once we can get rid of the WSACreateEvent.
>> May be better to explain that this is due to SocketServer dependency
>>
>
>
>
>
http://codereview.chromium.**org/9463023/<http://codereview.chromium.org/9463...
>

[Sergey Ulanov, 2012-02-24 20:45:11]

On Fri, Feb 24, 2012 at 12:39 PM, <cpu@chromium.org> wrote:

> You don't plan on opening tcp/udp sockets from a renderer, right?
>
No


>
>
http://codereview.chromium.**org/9463023/<http://codereview.chromium.org/9463...
>

[jschuh, 2012-02-24 20:51:48]

On 2012/02/24 20:44:53, sergeyu wrote:
> On Fri, Feb 24, 2012 at 12:38 PM, <mailto:jschuh@chromium.org> wrote:
> 
> > Sorry for the drive-by, but Winsock is absolutely forbidden in the renderer
> > process. If you initialize it a compromised renderer will have full access
> > to
> > the network device, which is a security regression we cannot allow.
> 
> 
> We don't really use winsock in the renderer (not creating any sockets),
> it's just that some code in libjingle depends on winsock. It worked fine
> previously, but stopped after winsock dependency was dropped from ffmpeg
> dlls. We'll need to fix libjingle to not depend on winsock APIs, but until
> that's done we need this fix to keep webrtc and remoting working.

It doesn't matter if you use it or not. Once you initialize it you've poked a
hole in the sandbox that cannot be closed. The fact that you were previously
relying on something else to poke the hole matters only to the extent that we
may may now have to ship a patch to close the security regression if it's
present in stable.

> 
> 
> >
> >
> >
> > On 2012/02/24 20:26:43, sergeyu wrote:
> >
> >> LGTM
> >>
> >
> >
> > http://codereview.chromium.**org/9463023/diff/1/chrome/**
> >
>
renderer/chrome_render_**process_observer.cc<http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_process_observer.cc>
> >
> >> File chrome/renderer/chrome_render_**process_observer.cc (right):
> >>
> >
> >
> > http://codereview.chromium.**org/9463023/diff/1/chrome/**
> >
>
renderer/chrome_render_**process_observer.cc#newcode225<http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_process_observer.cc#newcode225>
> >
> >> chrome/renderer/chrome_render_**process_observer.cc:225: // ws2_32.dll
> >> is used
> >>
> > by
> >
> >> JingleThreadWrapper for WebRTC and Chromoting.
> >> nit: add an empty line above this comment.
> >>
> >
> >
> > http://codereview.chromium.**org/9463023/diff/1/chrome/**
> >
>
renderer/chrome_render_**process_observer.cc#newcode226<http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_process_observer.cc#newcode226>
> >
> >> chrome/renderer/chrome_render_**process_observer.cc:226: //
> >> TODO(ronghuawu):
> >> Remove this once we can get rid of the WSACreateEvent.
> >> May be better to explain that this is due to SocketServer dependency
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/9463023/%3Chttp://codereview.chromium.org/94...>
> >

[tommi (sloooow) - chröme, 2012-02-24 21:00:56]

Please also file a p1 bug to get this fixed and mention the bug id in the
todo.
On Feb 24, 2012 9:51 PM, <jschuh@chromium.org> wrote:

> On 2012/02/24 20:44:53, sergeyu wrote:
>
>> On Fri, Feb 24, 2012 at 12:38 PM, <mailto:jschuh@chromium.org> wrote:
>>
>
>  > Sorry for the drive-by, but Winsock is absolutely forbidden in the
>> renderer
>> > process. If you initialize it a compromised renderer will have full
>> access
>> > to
>> > the network device, which is a security regression we cannot allow.
>>
>
>
>  We don't really use winsock in the renderer (not creating any sockets),
>> it's just that some code in libjingle depends on winsock. It worked fine
>> previously, but stopped after winsock dependency was dropped from ffmpeg
>> dlls. We'll need to fix libjingle to not depend on winsock APIs, but until
>> that's done we need this fix to keep webrtc and remoting working.
>>
>
> It doesn't matter if you use it or not. Once you initialize it you've
> poked a
> hole in the sandbox that cannot be closed. The fact that you were
> previously
> relying on something else to poke the hole matters only to the extent that
> we
> may may now have to ship a patch to close the security regression if it's
> present in stable.
>
>
>
>  >
>> >
>> >
>> > On 2012/02/24 20:26:43, sergeyu wrote:
>> >
>> >> LGTM
>> >>
>> >
>> >
>> > http://codereview.chromium.****org/9463023/diff/1/chrome/**
>> >
>>
>
> renderer/chrome_render_****process_observer.cc<http://**
> codereview.chromium.org/**9463023/diff/1/chrome/**renderer/chrome_render_*
>
*process_observer.cc<http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_process_observer.cc>
> >
>
>> >
>> >> File chrome/renderer/chrome_render_****process_observer.cc (right):
>> >>
>> >
>> >
>> > http://codereview.chromium.****org/9463023/diff/1/chrome/**
>> >
>>
>
> renderer/chrome_render_****process_observer.cc#**newcode225<
> http://codereview.**chromium.org/9463023/diff/1/**
>
chrome/renderer/chrome_render_**process_observer.cc#newcode225<http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_process_observer.cc#newcode225>
> **>
>
>> >
>> >> chrome/renderer/chrome_render_****process_observer.cc:225: //
>> ws2_32.dll
>> >> is used
>> >>
>> > by
>> >
>> >> JingleThreadWrapper for WebRTC and Chromoting.
>> >> nit: add an empty line above this comment.
>> >>
>> >
>> >
>> > http://codereview.chromium.****org/9463023/diff/1/chrome/**
>> >
>>
>
> renderer/chrome_render_****process_observer.cc#**newcode226<
> http://codereview.**chromium.org/9463023/diff/1/**
>
chrome/renderer/chrome_render_**process_observer.cc#newcode226<http://codereview.chromium.org/9463023/diff/1/chrome/renderer/chrome_render_process_observer.cc#newcode226>
> **>
>
>> >
>> >> chrome/renderer/chrome_render_****process_observer.cc:226: //
>> >> TODO(ronghuawu):
>> >> Remove this once we can get rid of the WSACreateEvent.
>> >> May be better to explain that this is due to SocketServer dependency
>> >>
>> >
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/9463023/%3Chttp://coderevi**
> ew.chromium.org/9463023/ <http://codereview.chromium.org/9463023/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/9463023/<http://codereview.chromium.org/9463...
>

[Sergey Ulanov, 2012-02-24 21:10:29]

On 2012/02/24 20:51:48, Justin Schuh wrote:
> On 2012/02/24 20:44:53, sergeyu wrote:
> > On Fri, Feb 24, 2012 at 12:38 PM, <mailto:jschuh@chromium.org> wrote:
> > 
> > > Sorry for the drive-by, but Winsock is absolutely forbidden in the
renderer
> > > process. If you initialize it a compromised renderer will have full access
> > > to
> > > the network device, which is a security regression we cannot allow.
> > 
> > 
> > We don't really use winsock in the renderer (not creating any sockets),
> > it's just that some code in libjingle depends on winsock. It worked fine
> > previously, but stopped after winsock dependency was dropped from ffmpeg
> > dlls. We'll need to fix libjingle to not depend on winsock APIs, but until
> > that's done we need this fix to keep webrtc and remoting working.
> 
> It doesn't matter if you use it or not. Once you initialize it you've poked a
> hole in the sandbox that cannot be closed. The fact that you were previously
> relying on something else to poke the hole matters only to the extent that we
> may may now have to ship a patch to close the security regression if it's
> present in stable.
 
My understanding is that even if the library is loaded it doesn't mean that the
process can create sockets, i.e. whether the library is loaded or not is
orthogonal to the socket permissions of the process. We are neither poking any
holes nor are we planning to. The problem is just some code still needs to be
able to call WSACreateEvent() windows API. That code regressed after DLL
dependency was removed and it now crashes when trying to resolve that symbol.

[jschuh, 2012-02-24 22:13:08]

On 2012/02/24 21:10:29, sergeyu wrote:
> On 2012/02/24 20:51:48, Justin Schuh wrote:
> > On 2012/02/24 20:44:53, sergeyu wrote:
> > > On Fri, Feb 24, 2012 at 12:38 PM, <mailto:jschuh@chromium.org> wrote:
> > > 
> > > > Sorry for the drive-by, but Winsock is absolutely forbidden in the
> renderer
> > > > process. If you initialize it a compromised renderer will have full
access
> > > > to
> > > > the network device, which is a security regression we cannot allow.
> > > 
> > > 
> > > We don't really use winsock in the renderer (not creating any sockets),
> > > it's just that some code in libjingle depends on winsock. It worked fine
> > > previously, but stopped after winsock dependency was dropped from ffmpeg
> > > dlls. We'll need to fix libjingle to not depend on winsock APIs, but until
> > > that's done we need this fix to keep webrtc and remoting working.
> > 
> > It doesn't matter if you use it or not. Once you initialize it you've poked
a
> > hole in the sandbox that cannot be closed. The fact that you were previously
> > relying on something else to poke the hole matters only to the extent that
we
> > may may now have to ship a patch to close the security regression if it's
> > present in stable.
>  
> My understanding is that even if the library is loaded it doesn't mean that
the
> process can create sockets, i.e. whether the library is loaded or not is
> orthogonal to the socket permissions of the process. We are neither poking any
> holes nor are we planning to. The problem is just some code still needs to be
> able to call WSACreateEvent() windows API. That code regressed after DLL
> dependency was removed and it now crashes when trying to resolve that symbol.

It's not whether or not the library is loaded. It's whether or not WSAStartup is
called, which initializes the network to the point that a compromised renderer
can issue IOCTLs directly. OTS is safe because it never calls WSAStartup
(although we should really eliminate the dependency). It sounds like ffmpeg was
unsafe, but I'll have to look into it more to verify. The usage here, however,
appears totally unsafe.

[Ronghua Wu (Left Chromium), 2012-02-24 22:18:51]

https://chromiumcodereview.appspot.com/9463023/diff/1/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_process_observer.cc (right):

https://chromiumcodereview.appspot.com/9463023/diff/1/chrome/renderer/chrome_...
chrome/renderer/chrome_render_process_observer.cc:225: // ws2_32.dll is used by
JingleThreadWrapper for WebRTC and Chromoting.
On 2012/02/24 20:26:43, sergeyu wrote:
> nit: add an empty line above this comment.

Done.

https://chromiumcodereview.appspot.com/9463023/diff/1/chrome/renderer/chrome_...
chrome/renderer/chrome_render_process_observer.cc:226: // TODO(ronghuawu):
Remove this once we can get rid of the WSACreateEvent.
On 2012/02/24 20:26:43, sergeyu wrote:
> May be better to explain that this is due to SocketServer dependency

Done.

[Sergey Ulanov, 2012-02-24 22:35:37]

On 2012/02/24 22:13:08, Justin Schuh wrote:
> On 2012/02/24 21:10:29, sergeyu wrote:
> > On 2012/02/24 20:51:48, Justin Schuh wrote:
> > > On 2012/02/24 20:44:53, sergeyu wrote:
> > > > On Fri, Feb 24, 2012 at 12:38 PM, <mailto:jschuh@chromium.org> wrote:
> > > > 
> > > > > Sorry for the drive-by, but Winsock is absolutely forbidden in the
> > renderer
> > > > > process. If you initialize it a compromised renderer will have full
> access
> > > > > to
> > > > > the network device, which is a security regression we cannot allow.
> > > > 
> > > > 
> > > > We don't really use winsock in the renderer (not creating any sockets),
> > > > it's just that some code in libjingle depends on winsock. It worked fine
> > > > previously, but stopped after winsock dependency was dropped from ffmpeg
> > > > dlls. We'll need to fix libjingle to not depend on winsock APIs, but
until
> > > > that's done we need this fix to keep webrtc and remoting working.
> > > 
> > > It doesn't matter if you use it or not. Once you initialize it you've
poked
> a
> > > hole in the sandbox that cannot be closed. The fact that you were
previously
> > > relying on something else to poke the hole matters only to the extent that
> we
> > > may may now have to ship a patch to close the security regression if it's
> > > present in stable.
> >  
> > My understanding is that even if the library is loaded it doesn't mean that
> the
> > process can create sockets, i.e. whether the library is loaded or not is
> > orthogonal to the socket permissions of the process. We are neither poking
any
> > holes nor are we planning to. The problem is just some code still needs to
be
> > able to call WSACreateEvent() windows API. That code regressed after DLL
> > dependency was removed and it now crashes when trying to resolve that
symbol.
> 
> It's not whether or not the library is loaded. It's whether or not WSAStartup
is
> called, which initializes the network to the point that a compromised renderer
> can issue IOCTLs directly. OTS is safe because it never calls WSAStartup
> (although we should really eliminate the dependency). It sounds like ffmpeg
was
> unsafe, but I'll have to look into it more to verify. The usage here, however,
> appears totally unsafe.

The code in this change just loads the library. It doesn't call WSAStartup().
And ffmpeg didn't call it. WSAStartup() may be called later, but only when
sandboxed. It was that way for long time and nothing has changed in how and
where WSAStartup() is called. It was never possible to create a sockets inside
sandbox on windows. That having said, I'm not an expert in winsocks or how the
sandbox works on windows. We should verify that it is still not possible to open
sockets from inside the sandbox.

[tommi (sloooow) - chröme, 2012-02-24 22:47:54]

Note that although the code in this change doesn't call WSAStartup, it does
add ws2_32.dll which is the DLL we were trying to load when we crashed.
Once the DLL is back in, we enable code that relies on ws2_32.

I quickly looked at the code, and it looks to me that if
physicalsocketserver.cc is linked into chrome,
then that pulls in a dependency on win32socketinit.cc.  win32socketinit.cc
in turn has a global variable
that initializes winsock by calling WSAStartup.

I did a quick check to see if that obj file is compiled, and it is.  I
didn't verify though if it gets pulled into Chrome (hey, it's Friday night
over here!).
Can you check?

On Fri, Feb 24, 2012 at 11:35 PM, <sergeyu@chromium.org> wrote:

> On 2012/02/24 22:13:08, Justin Schuh wrote:
>
>> On 2012/02/24 21:10:29, sergeyu wrote:
>> > On 2012/02/24 20:51:48, Justin Schuh wrote:
>> > > On 2012/02/24 20:44:53, sergeyu wrote:
>> > > > On Fri, Feb 24, 2012 at 12:38 PM, <mailto:jschuh@chromium.org>
>> wrote:
>> > > >
>> > > > > Sorry for the drive-by, but Winsock is absolutely forbidden in the
>> > renderer
>> > > > > process. If you initialize it a compromised renderer will have
>> full
>> access
>> > > > > to
>> > > > > the network device, which is a security regression we cannot
>> allow.
>> > > >
>> > > >
>> > > > We don't really use winsock in the renderer (not creating any
>> sockets),
>> > > > it's just that some code in libjingle depends on winsock. It worked
>> fine
>> > > > previously, but stopped after winsock dependency was dropped from
>> ffmpeg
>> > > > dlls. We'll need to fix libjingle to not depend on winsock APIs, but
>>
> until
>
>> > > > that's done we need this fix to keep webrtc and remoting working.
>> > >
>> > > It doesn't matter if you use it or not. Once you initialize it you've
>>
> poked
>
>> a
>> > > hole in the sandbox that cannot be closed. The fact that you were
>>
> previously
>
>> > > relying on something else to poke the hole matters only to the extent
>> that
>> we
>> > > may may now have to ship a patch to close the security regression if
>> it's
>> > > present in stable.
>> >
>> > My understanding is that even if the library is loaded it doesn't mean
>> that
>> the
>> > process can create sockets, i.e. whether the library is loaded or not is
>> > orthogonal to the socket permissions of the process. We are neither
>> poking
>>
> any
>
>> > holes nor are we planning to. The problem is just some code still needs
>> to
>>
> be
>
>> > able to call WSACreateEvent() windows API. That code regressed after DLL
>> > dependency was removed and it now crashes when trying to resolve that
>>
> symbol.
>
>  It's not whether or not the library is loaded. It's whether or not
>> WSAStartup
>>
> is
>
>> called, which initializes the network to the point that a compromised
>> renderer
>> can issue IOCTLs directly. OTS is safe because it never calls WSAStartup
>> (although we should really eliminate the dependency). It sounds like
>> ffmpeg
>>
> was
>
>> unsafe, but I'll have to look into it more to verify. The usage here,
>> however,
>> appears totally unsafe.
>>
>
> The code in this change just loads the library. It doesn't call
> WSAStartup().
> And ffmpeg didn't call it. WSAStartup() may be called later, but only when
> sandboxed. It was that way for long time and nothing has changed in how and
> where WSAStartup() is called. It was never possible to create a sockets
> inside
> sandbox on windows. That having said, I'm not an expert in winsocks or how
> the
> sandbox works on windows. We should verify that it is still not possible
> to open
> sockets from inside the sandbox.
>
>
http://codereview.chromium.**org/9463023/<http://codereview.chromium.org/9463...
>

[Sergey Ulanov, 2012-02-24 22:52:45]

On 2012/02/24 22:47:54, tommi wrote:
> Note that although the code in this change doesn't call WSAStartup, it does
> add ws2_32.dll which is the DLL we were trying to load when we crashed.
> Once the DLL is back in, we enable code that relies on ws2_32.
> 
> I quickly looked at the code, and it looks to me that if
> physicalsocketserver.cc is linked into chrome,
> then that pulls in a dependency on win32socketinit.cc.  win32socketinit.cc
> in turn has a global variable
> that initializes winsock by calling WSAStartup.
> 
> I did a quick check to see if that obj file is compiled, and it is.  I
> didn't verify though if it gets pulled into Chrome (hey, it's Friday night
> over here!).
> Can you check?

Yes, it is still being created for each talk_base::Thread object, but in Chrome
it is never used to create sockets. We should fix it in libjingle, i.e. remove
this dependency and make it possible to create talk_base::Thread objects without
a socket server.

> 
> On Fri, Feb 24, 2012 at 11:35 PM, <mailto:sergeyu@chromium.org> wrote:
> 
> > On 2012/02/24 22:13:08, Justin Schuh wrote:
> >
> >> On 2012/02/24 21:10:29, sergeyu wrote:
> >> > On 2012/02/24 20:51:48, Justin Schuh wrote:
> >> > > On 2012/02/24 20:44:53, sergeyu wrote:
> >> > > > On Fri, Feb 24, 2012 at 12:38 PM, <mailto:jschuh@chromium.org>
> >> wrote:
> >> > > >
> >> > > > > Sorry for the drive-by, but Winsock is absolutely forbidden in the
> >> > renderer
> >> > > > > process. If you initialize it a compromised renderer will have
> >> full
> >> access
> >> > > > > to
> >> > > > > the network device, which is a security regression we cannot
> >> allow.
> >> > > >
> >> > > >
> >> > > > We don't really use winsock in the renderer (not creating any
> >> sockets),
> >> > > > it's just that some code in libjingle depends on winsock. It worked
> >> fine
> >> > > > previously, but stopped after winsock dependency was dropped from
> >> ffmpeg
> >> > > > dlls. We'll need to fix libjingle to not depend on winsock APIs, but
> >>
> > until
> >
> >> > > > that's done we need this fix to keep webrtc and remoting working.
> >> > >
> >> > > It doesn't matter if you use it or not. Once you initialize it you've
> >>
> > poked
> >
> >> a
> >> > > hole in the sandbox that cannot be closed. The fact that you were
> >>
> > previously
> >
> >> > > relying on something else to poke the hole matters only to the extent
> >> that
> >> we
> >> > > may may now have to ship a patch to close the security regression if
> >> it's
> >> > > present in stable.
> >> >
> >> > My understanding is that even if the library is loaded it doesn't mean
> >> that
> >> the
> >> > process can create sockets, i.e. whether the library is loaded or not is
> >> > orthogonal to the socket permissions of the process. We are neither
> >> poking
> >>
> > any
> >
> >> > holes nor are we planning to. The problem is just some code still needs
> >> to
> >>
> > be
> >
> >> > able to call WSACreateEvent() windows API. That code regressed after DLL
> >> > dependency was removed and it now crashes when trying to resolve that
> >>
> > symbol.
> >
> >  It's not whether or not the library is loaded. It's whether or not
> >> WSAStartup
> >>
> > is
> >
> >> called, which initializes the network to the point that a compromised
> >> renderer
> >> can issue IOCTLs directly. OTS is safe because it never calls WSAStartup
> >> (although we should really eliminate the dependency). It sounds like
> >> ffmpeg
> >>
> > was
> >
> >> unsafe, but I'll have to look into it more to verify. The usage here,
> >> however,
> >> appears totally unsafe.
> >>
> >
> > The code in this change just loads the library. It doesn't call
> > WSAStartup().
> > And ffmpeg didn't call it. WSAStartup() may be called later, but only when
> > sandboxed. It was that way for long time and nothing has changed in how and
> > where WSAStartup() is called. It was never possible to create a sockets
> > inside
> > sandbox on windows. That having said, I'm not an expert in winsocks or how
> > the
> > sandbox works on windows. We should verify that it is still not possible
> > to open
> > sockets from inside the sandbox.
> >
> >
>
http://codereview.chromium.**org/9463023/%3Chttp://codereview.chromium.org/94...>
> >

[tommi (sloooow) - chröme, 2012-02-24 23:00:41]

No question, we need to remove it. But if I understand Carlos correctly, it
doesn't matter whether or not we create sockets:
  "It's not whether or not the library is loaded. It's whether or not
WSAStartup is called"

So, assuming that's right, we have to do more than just this CL in order to
fix the problem. Even in the short term.

We already have an override for winsock32init.cc.  Can we add to this CL a
change to it that changes EnsureWinsockInit() to be empty?
I.e. remove the call to net::EnsureWinsockInit()?

[Sergey Ulanov, 2012-02-24 23:00:49]

On 2012/02/24 22:47:54, tommi wrote:
> Note that although the code in this change doesn't call WSAStartup, it does
> add ws2_32.dll which is the DLL we were trying to load when we crashed.
> Once the DLL is back in, we enable code that relies on ws2_32.
> 
> I quickly looked at the code, and it looks to me that if
> physicalsocketserver.cc is linked into chrome,
> then that pulls in a dependency on win32socketinit.cc.  win32socketinit.cc
> in turn has a global variable
> that initializes winsock by calling WSAStartup.

We have special version of that file in the overrides:
third_party/libjingle/overrides/talk/base/win32socketinit.cc . It doesn't have
WindowsInitializer class. Still, you are right that physicalsocketserver.cc
calls EnusreWinsockInitialized() which calls WSAStartup(). The point that this
happens later when the process is already sandboxed, so I think it should be
safe, and the renderer process should not be able to use winsock. After all, if
calling WSAStartup() inside the sandbox was opening a hole in the sandbox then
malicious code would be able to use that function too.

[Sergey Ulanov, 2012-02-24 23:03:26]

On 2012/02/24 23:00:41, tommi wrote:
> No question, we need to remove it. But if I understand Carlos correctly, it
> doesn't matter whether or not we create sockets:
>   "It's not whether or not the library is loaded. It's whether or not
> WSAStartup is called"

Sorry I missed the actual question the first time. See my answer in the previous
message.

> 
> So, assuming that's right, we have to do more than just this CL in order to
> fix the problem. Even in the short term.
> 
> We already have an override for winsock32init.cc.  Can we add to this CL a
> change to it that changes EnsureWinsockInit() to be empty?
> I.e. remove the call to net::EnsureWinsockInit()?

We can. That would break some unittests that rely on it, but it should be easy
to fix by calling net::EnsureWinsockInit() explicitly in the tests.

[cpu_(ooo_6.6-7.5), 2012-02-24 23:22:10]

WSAStartup should be failing if the sandbox is locked down. Is it not?

If it is not then I am even more concerned. It means that some of the
initialization moved to DllMain in the past 3 years.

Just to be clear: sockets on windows are a bsd construct and have no ACLs, so
there is no way to prevent a renderer to open them _except_ that to be able to
actually use them you need to load a bunch of configuration from the registry,
(and depending on the machine configuration, some other dlls), you can't guess
that stuff up. By allowing winsock initialization, the configuration is now in
memory and now you just need the right ioctls to open sockets.

[Sergey Ulanov, 2012-02-25 00:39:09]

On 2012/02/24 23:22:10, cpu wrote:
> WSAStartup should be failing if the sandbox is locked down. Is it not?

Probably it does fail, but WinsockInitSingleton just ignores that error.

> If it is not then I am even more concerned. It means that some of the
> initialization moved to DllMain in the past 3 years.
> 
> Just to be clear: sockets on windows are a bsd construct and have no ACLs, so
> there is no way to prevent a renderer to open them _except_ that to be able to
> actually use them you need to load a bunch of configuration from the registry,
> (and depending on the machine configuration, some other dlls), you can't guess
> that stuff up. By allowing winsock initialization, the configuration is now in
> memory and now you just need the right ioctls to open sockets.

[Ronghua Wu (Left Chromium), 2012-02-25 08:08:28]

On 2012/02/25 00:39:09, sergeyu wrote:
> On 2012/02/24 23:22:10, cpu wrote:
> > WSAStartup should be failing if the sandbox is locked down. Is it not?
> 
> Probably it does fail, but WinsockInitSingleton just ignores that error.
> 
> > If it is not then I am even more concerned. It means that some of the
> > initialization moved to DllMain in the past 3 years.
> > 
> > Just to be clear: sockets on windows are a bsd construct and have no ACLs,
so
> > there is no way to prevent a renderer to open them _except_ that to be able
to
> > actually use them you need to load a bunch of configuration from the
registry,
> > (and depending on the machine configuration, some other dlls), you can't
guess
> > that stuff up. By allowing winsock initialization, the configuration is now
in
> > memory and now you just need the right ioctls to open sockets.

If we may confirm that WSAStartup will fail and we can't create and connect
sockets inside  render process. Can we consider land this cl as the short term
solution?

I also tried to remove the dependencies in the below cl, but it seems more and
more files are getting involved and may take longer than we thought:
https://chromiumcodereview.appspot.com/9455070/

[cpu_(ooo_6.6-7.5), 2012-02-28 05:02:25]

Let me talk to the security folks, what thing we can check to feel ok with this
change.

[Ronghua Wu (Left Chromium), 2012-03-05 18:59:14]

Closing this one as we fixed the problem by removing the dependencies.