Commit graph

526 commits

Author SHA1 Message Date
Miklos Vajna
4dbdd72bc2 wsd: avoid std::string::compare() in FileServer
When we are just interested in equality. compare() is more meant for
sorting functions where negative/zero/positive return value is useful.

Change-Id: I11138a14dc08e23d33f3848aeb734d9f56f3e9f7
2017-04-12 13:46:09 +02:00
Ashod Nakashian
1752dd74d6 wsd: avoid miscounting outstanding child forks
The number of outstanding child forks can
become negative if more children are
spawned than requested.

This prevents such a scenario from
permanently preventing WSD from spawning
new children, which happens when
OutstandingForks is negative.

Change-Id: Ief1e56d7b4a079e097ca2d18bd90a01d935f6b30
Reviewed-on: https://gerrit.libreoffice.org/36437
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-12 06:11:05 +02:00
Miklos Vajna
08989a12ac wsd: avoid use-after-free in ClientSession
Commit 1e1f23716c fixed this already by
introducing by-value parameters, but
8a1f321c84 broke it. Fix this again, this
time more explicitly.

Change-Id: If29250ac2e99855796935b5cc05ccb222f8a4ad5
Reviewed-on: https://gerrit.libreoffice.org/36436
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Tested-by: Michael Meeks <michael.meeks@collabora.com>
2017-04-11 23:20:08 +02:00
Miklos Vajna
8a1f321c84 DocumentBroker: avoid unnecessary copying
Change-Id: Iaa555ed8e347d0e1712c617839f007d0b4f3204b
2017-04-11 08:54:28 +02:00
Pranav Kant
4d6b338bf0 security: Stricter Referrer-Policy: no-referrer
I don't think we should leak our address
(which mostly is behind a WOPI host and end-user
has no idea of what host LibreOffice Online is running at) in the
Referer header. Lets be more strict here and don't leak our address
at all.

Change-Id: Ibc30e9b64e2e06e2e8d541c5f089320ecb11412b
2017-04-11 00:02:00 +05:30
Jan Holesovsky
e0f7c3fc67 Set the _owner even in the release builds.
We are warning about thread affinity even in the non-debug builds.

Change-Id: Ia91170765e9f4a29939dee847899345e9396d2c3
2017-04-10 14:55:04 +02:00
Pranav Kant
1437a060ec security: Implement HTTP Public key pinning
Though this guard the user against MITM attacks, but enabling this also
has the potential to brick your websites. So, do not use it/enable it
without understanding what it actually is.

See: https://developer.mozilla.org/en-US/docs/Web/HTTP/Public_Key_Pinning

Though this should work, but I have not been able to test it because of
Firefox and Chrome's limitation/feature that key validation is not done
when certificate chain terminates at a user-defined trust anchor and I
couldn't find any way to temporarily enable the HPKP key validation for
such CA chains.

Change-Id: I64d4ff82b04c59642fa7b8bac2f8788a03950b28
Reviewed-on: https://gerrit.libreoffice.org/36357
Reviewed-by: pranavk <pranavk@collabora.co.uk>
Tested-by: pranavk <pranavk@collabora.co.uk>
2017-04-10 14:46:24 +02:00
Pranav Kant
74020e0f1f Revert "wsd: Fileserver cleanup"
This reverts commit de2bc17c04af088d9c7e18a97216b174494e1a9c.

Lets not introduce any cleanup commits while we are near a release, will
apply it again after the release. The cleanup is supposed to not handle
the custom file server root correctly, so don't forget to test it with
a custom file server root before re-reverting.

It changes the path where loleaflet.html is searched for from
/usr/share/loolwsd/loleaflet/... to /usr/share/loleaflet/...
and doesn't find it there.

Change-Id: I23940e9a3e06721f0a8b7493a526f42d2072cfa4
2017-04-10 15:26:05 +05:30
Pranav Kant
a0d7c33877 security: X-Frame-Options: Deny framing if no wopi host
Change-Id: I6936f8a11e3e076e111e0883305f47064e032983
2017-04-10 15:26:00 +05:30
Miklos Vajna
8958e1c767 wsd: make requestURI a const reference
It's copy-constructed from a const reference but is only used as const
reference.

Change-Id: I9a58561616bcfeff0c45803f3244f8e78d54731a
2017-04-10 10:44:14 +02:00
Ashod Nakashian
fa2e2869cf wsd: logging cleanups
Change-Id: Ia06bc5b1e0090c8198ac4ba2b88d5e57f8e2b168
Reviewed-on: https://gerrit.libreoffice.org/36327
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-10 06:11:58 +02:00
Ashod Nakashian
9a761ffe68 wsd: clear the incoming buffer before upgrading to WS
There was an interesting race when we cleared the
inBuffer after the WS upgrade. Since during the
upgrade we also transfer the socket to the DocBroker,
which has its own poll thread, the DocBroker poll
could trigger a POLLIN event if data comes
while the handler (that is handling the WS upgrad
and transfer to DocBroker) hasn't got to the point
where it clears the inBuffer of the data we just
read (i.e. the HTTP GET request). Even if not
the case, after transfering a socket to another
poll thread the socket buffers should not be
touched.

Here we move the inBuffer clearing to be as soon
as we have successfully parsed the request and
are ready to process it.

Also, we don't clear the full buffer, in case
we had read into the buffer both the requst
and the first message, if the thread was switched
out right after getting the POLLIN but before
reading from the socket, giving enough time to
receive more data and reading it together with
first read (which is the request).

Change-Id: I9888d4c2b70d2e433824818bbe7f69f13742486c
Reviewed-on: https://gerrit.libreoffice.org/36326
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-10 06:11:06 +02:00
Ashod Nakashian
bc41ad9bf9 wsd: remove outdated comment and simplify
Change-Id: I47e8b22708ab64ad95aa681407344686e6d4eb9d
Reviewed-on: https://gerrit.libreoffice.org/36325
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-10 06:10:30 +02:00
Ashod Nakashian
679a39eb0b wsd: send recycling message to clients before going down
Change-Id: I388ca55524983d554fabf247bb3baee23010657d
Reviewed-on: https://gerrit.libreoffice.org/36329
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-10 06:08:59 +02:00
Pranav Kant
1ca873d57e security: X-XSS-Protection header
Change-Id: I050cba3ad8aeedaefa773d78254a3a37a7ddef30
2017-04-09 23:32:06 +05:30
Pranav Kant
61b7112aa7 security: X-Content-Type-Options: nosniff
Don't think it is necessary/useful to have this header at other places.
This is the most important and perhaps the only where presence of this
header is required and seems sensible to prevent potential attacks.

Change-Id: Iad318e4b83264ac83620b86a40a49e7384e4015e
2017-04-09 23:32:06 +05:30
Pranav Kant
49bd32c630 security: CORS: No need for this header
No idea why it was here in the first place, but download requests are
only made from frames with same origin, so there should be no need to
specify such headers which allow anyone (with other origins) to make
download requests to us.

Change-Id: I314a7ad4c6df8664b1d191cb88ae42c4248ff517
2017-04-09 23:32:06 +05:30
Pranav Kant
32dde923f7 security: CORS: No need to allow requests from anywhere
insertfile post requests should be made only from our origin.
Mentioning a '*' against allow-access-allow-origin allows other origins
to be able to make requests to insertfile too provided the attacker
knows the doc key which is not very hard to guess/get.

Change-Id: If98351df48935cfcdc18d6879167c0ac6089796c
2017-04-09 23:32:06 +05:30
Pranav Kant
df8ac5f33e wsd: Only set these headers if its WOPI
Change-Id: I1ccedc9828a724b55f8642aaa2b934c37f49a4dd
2017-04-09 23:32:06 +05:30
Michael Meeks
254de88a58 Clear ownership of socket while it is being transferred.
This addresses a gap between ServerSocket accepting new sockets,
and their being added to their new polls.
2017-04-07 20:59:34 +01:00
Pranav Kant
1a1a3ebb3c wsd: Fileserver cleanup
Remove unnecessary checks

Rename preprocessFile -> preprocessAndSendLoleafletHtml and
Rename isAdminLoggedIn -> tryAdminLogin
so that their name matches the actual reality of what these
function really does.

Change-Id: I549eae31f8ab0a320bb3ff8ecd17a282b8f91e1a
2017-04-07 13:46:04 +05:30
Pranav Kant
1614f8d417 security: Mention X-Frame-Options too for ie/edge
ie/edge ignores frame-ancestor directive of CSP (yet). Mention X-Frame-Options
for them. Similary, X-Frame-Options allow-from attribute is not
supported by Chrome:
(see https://bugs.chromium.org/p/chromium/issues/detail?id=511521)
In that case, we already have frame-ancestor CSP directive for it.

Change-Id: Ide00c4db88c438de5e9c679360b3da6f4eb4a1be
2017-04-07 13:46:04 +05:30
Pranav Kant
ffc5d516b4 security: CSP: Add frame-ancestor directive
Block embedding LibreOffice Online is frames of different origin.

Change-Id: If3e04a0704e42853dc757b4be1f30fc22b8b33e4
2017-04-07 13:46:04 +05:30
Ashod Nakashian
37f499a7f6 wsd: merge DocumentBroker poll exit conditions
These conditions must be checked together. Otherwise we might
set _stop prematurely.

Change-Id: I3de0d2b3833959593315669ad245f94c1243f7f7
Reviewed-on: https://gerrit.libreoffice.org/36242
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-07 04:47:29 +00:00
Ashod Nakashian
bb12de8035 wsd: lower the max number of test docs and connections
Tests should have sensible limits so they don't
go overboard and fail needlessly causing noise.

Change-Id: Idd556c348cc0e97e38c710fdbf76fe20c76d8f9b
Reviewed-on: https://gerrit.libreoffice.org/36241
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-07 04:41:30 +00:00
Ashod Nakashian
1e1f23716c wsd: don't take reference to session member being destroyed
Change-Id: I0074f4557018feb47a7a2a95a3fca238407a0023
Reviewed-on: https://gerrit.libreoffice.org/36227
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 17:58:47 +00:00
Ashod Nakashian
7da1909d3c wsd: kill DocumentBroker::getSessionsCount
Change-Id: Icd3229fe9b7d2f17a0e8a8f955c41ead8bca98c7
Reviewed-on: https://gerrit.libreoffice.org/36226
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 17:55:24 +00:00
Michael Meeks
90127ac0e3 Let the DocBroker thread clean itself up and expire.
(cherry picked from commit 2e372b70b32d4e052458547daa229c537442774f)

Change-Id: I5835c83f44ef770fa6ccd2418fc6ca73e17694e4
Reviewed-on: https://gerrit.libreoffice.org/36225
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 17:53:48 +00:00
Michael Meeks
3d945a5c38 Revert "Don't cleanup DocumentBrokers that still have their thread running."
This reverts commit df8dc43be4.

DocumentBroker::isAlive already checks _threadFinished.
2017-04-06 16:35:55 +01:00
Michael Meeks
df8dc43be4 Don't cleanup DocumentBrokers that still have their thread running.
Plenty of time to do that next time around the cleanup.
We should still, really be doing the majority of the timeout work
inside the DocumentBroker poll itself.
2017-04-06 16:24:54 +01:00
Jan Holesovsky
fa042ed0e3 Make the callgrinding possible again.
Change-Id: I9e8e0e3d088c4af29f2701a0318a508f14327fff
2017-04-06 10:22:38 +02:00
Ashod Nakashian
3d03a0fb5d wsd: accomodate accept_poll shutdown
When shutting down accept_poll from
main, we can't remove sockets or cleanup.
That work needs to be done fro within accept_poll's
thread. This is different from when DocBroker's
poll needs to cleanup its own sockets before
it exists.

So we split the stop and removeSockets so they
can each be called in the proper way.

For accept_poll and others that joinThread
we queue a callback to cleanup before stopping.

Change-Id: If780d6a97ac0fc6da6897f895d5b4dda443f9e73
Reviewed-on: https://gerrit.libreoffice.org/36186
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 07:01:18 +00:00
Ashod Nakashian
a22118df10 wsd: inhibit thread checks sooner when shutting down
LOOLWSDServer::stop() now removes the accept_poll
socket, which will assertCorrectThread. So we need
to disable checks before it.

Change-Id: I3445610c1c48c2b4c23bcfcbc87e236b36d18c0b
Reviewed-on: https://gerrit.libreoffice.org/36185
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 06:33:24 +00:00
Ashod Nakashian
5e4528b593 wsd: leave the poll running so DocBroker can flush the sockets
By stopping the poll we fail to notify the clients
of the shutdown. Let the DocBroker poll thread
take care of the poll stopping when it's ready.

Change-Id: I2cb4c76da2722ce41a60fc1983b10dc8b18b4cab
Reviewed-on: https://gerrit.libreoffice.org/36184
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 06:20:10 +00:00
Ashod Nakashian
4a7a0fb477 wsd: remove sockets when stopping poll thread
And assume correct thread if poll thread is
not running (i.e. no race).

Change-Id: I17958e682aba434ebb47fe0de199b9f530b54dee
Reviewed-on: https://gerrit.libreoffice.org/36183
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 06:19:06 +00:00
Michael Meeks
01519eff70 Always cleanup DocBrokers in the PrisonerPoll thread.
This simplifies things, and keeps process management in one thread.
Also - wakeup the DocumentBroker when we want to stop it.

Change-Id: I597ba4b34719fc072a4b4ad3697442b5eebe5784
Reviewed-on: https://gerrit.libreoffice.org/36182
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 06:16:44 +00:00
Ashod Nakashian
cbe6f0c813 wsd: move prisoner socket in the poll thread
Change-Id: I4097da97d4485d98618604c039a4570efe52bc19
Reviewed-on: https://gerrit.libreoffice.org/36181
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 06:12:39 +00:00
Michael Meeks
bb4459a288 Give up on doing thread checks during late shutdown.
Change-Id: Icb600e4d734e075bec6c2cf6adbb2afd58c0d98b
Reviewed-on: https://gerrit.libreoffice.org/36180
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-06 06:11:35 +00:00
Michael Meeks
737f7111b0 Set thread owner to zero earlier to avoid race.
Stop-gap fix, while we come up with a nice API for transferring
sockets between SocketPolls, and/or detaching them generally.
2017-04-05 21:01:48 +01:00
Michael Meeks
381bed9388 Remove redundant structure, include, and _stop members. 2017-04-05 18:06:58 +01:00
Michael Meeks
2d1764d30e Dump ClientSession and MessageQueue state too. 2017-04-05 17:59:29 +01:00
Michael Meeks
185540bcde Inhibit thread checks for SIGUSR1 handling.
USR1 handling is not thread-safe; we walk the structures and hope.
2017-04-05 17:58:52 +01:00
Jan Holesovsky
cb2b788cc7 assert(isCorrectThread()) -> assertCorrectThread().
assert()'s are no-op in the release builds, but we still want to see threading
problems in the log at least.

Change-Id: Idb02bb018e8f2d628a57ab570249613ad00bcff2
2017-04-05 14:49:30 +02:00
Michael Meeks
4b7dee5652 Remove un-used _stop member, and cleanup redundant code. 2017-04-05 11:57:11 +01:00
Ashod Nakashian
1ad4037dd7 wsd: allow for slow startup of LOK
Change-Id: Idf821f2a3638e76e1a8b169d5672a2059b00491c
Reviewed-on: https://gerrit.libreoffice.org/36118
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-05 04:48:00 +00:00
Ashod Nakashian
2254b71682 wsd: some informative logging
Change-Id: I4338f5bd8056d1d66da01efaa1a1fe54f8717793
Reviewed-on: https://gerrit.libreoffice.org/36116
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-05 04:46:52 +00:00
Ashod Nakashian
38f955b5c5 wsd: start DocBroker thread before adding callbacks
And move more into the callback to ensure
thread affinity.

Change-Id: I1d6985716d0d36aa488b65263ecb41f444f77255
Reviewed-on: https://gerrit.libreoffice.org/36115
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-05 04:46:22 +00:00
Ashod Nakashian
2576f9c4e9 wsd: correctly search for available prisoner port
Search for the next 100 ports for a usable one
and pass the one found to forkit so it connects
on that one instead of the default.

Change-Id: I26697dd8b5a35992f9e000a35ad5b44c3a3699dd
Reviewed-on: https://gerrit.libreoffice.org/36114
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-05 04:45:49 +00:00
Ashod Nakashian
e848996247 wsd: simplify career span timing
Change-Id: I0bfb3bca99f3f20ca9244e580c80801e89890fc2
Reviewed-on: https://gerrit.libreoffice.org/36113
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-05 04:45:23 +00:00
Ashod Nakashian
e0d6ab7835 wsd: stop poll threads before joining
Also add symmetric stopPrisoners to
match startPrisoners to LOOLWSDServer.

Change-Id: I78d76d86a8e7efc0964cd06df2340658c1b6c4ba
Reviewed-on: https://gerrit.libreoffice.org/36111
Reviewed-by: Ashod Nakashian <ashnakash@gmail.com>
Tested-by: Ashod Nakashian <ashnakash@gmail.com>
2017-04-05 04:43:22 +00:00