bugFreeciv - Bugs: bug #19729, luasql (database.lua) threat model...

 
 
Show feedback again

bug #19729: luasql (database.lua) threat model unclear, security measures get in the way

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Sat 12 May 2012 08:35:00 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalStatus: Need Info
Assigned to: NoneOpen/Closed: Open
Release: S2_4Operating System: Any
Planned Release: 2.6.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Sun 08 Jul 2012 05:23:34 PM UTC, comment #4:

> (Perhaps the path change should be in its own ticket, leaving
> this one for the wider discussion.)

I had to update the patch again, so I did create a new ticket: bug #19911.

Jacob Nevins <jtn>
Project Administrator
Sun 08 Jul 2012 12:07:41 AM UTC, comment #3:

Patch needed a little tweak; new (tested) ones attached.

So this installs to locations like /usr/local/etc/freeciv/database.lua. I guess that makes sense; we're saying the server operator might want to tweak this, so Debian would probably treat it as a conffile and put it in /etc (in fact I've updated README.packaging to suggest this).

I agree we can live without a way to override the "system" database.lua for now.

So, this seems like a reasonable position for 2.4, thanks. I can do something dirty to get round the lack of Lua os.time() and thus complete the SQLite support.

For 2.5, any objection to lifting some or all of the restrictions on the Lua instance used for luasql?

(Perhaps the path change should be in its own ticket, leaving this one for the wider discussion.)

(file #16014, file #16015)

Jacob Nevins <jtn>
Project Administrator
Thu 05 Jul 2012 03:17:04 AM UTC, comment #2:

Untested patch to have database.lua in $sysconfdir/freeciv/. Untested since my primary development machine is down and I don't have mysql setup on this one. Can someone test?

Note that since $sysconfdir/freeciv is the only location database.lua is looked from, it cannot be used directly from builddir.

(file #15976)

Marko Lindqvist <cazfi>
Project Administrator
Thu 05 Jul 2012 02:36:57 AM UTC, comment #1:

Lets start to open this by stating that database.lua should not be in data path. Most importantly it should not be searched like other data files (meaning that even without overwriting the original one, it would be possible to just place replacement on higher priority location), but it also should be in place where files are not usually written (unpacking custom modpack to datadir root or downloading it with freeciv-modpack should not be able to overwrite it) Server operators should create separate mysql user for freeciv that has no right to do anything else, but do they? In the worst case attacker's database.lua gets access to other, non-freeciv, databases on the server machine.

How important it's for non-root user to be able to write his/her own database.lua, if root has anyway installed freeciv? Remember that until 2.4 people have lived without ability to have their own database.lua at all. Lets make this most simple way for S2_4 by reading database.lua from one given location (${sysconfdir}/freeciv/database.lua?) If really needed, we can add support for overriding that database.lua in 2.5 with more time to test it before release. One option for such a database.lua location is ~/.freeciv/ root (freeciv-modpack downloads to version specific subdirs, or to scenarios subdir)

For beta1 we should have correct location for database.lua. Other things can wait after the beta, or?

Marko Lindqvist <cazfi>
Project Administrator
Sat 12 May 2012 08:35:00 PM UTC, original submission:

data/database.lua (the Lua script handling authentication and database access in 2.4+) is loaded in its own Lua instance, separate from the instance in which the older server-side "game"/"scenario" scripts run.

However, the "auth" Lua instance gets the same restricted environment that "scenario" scripts do (see bug #15624, bug #15725).

Right now this is a pain for me because for patch #3287, I want to call Lua's os.time() rather than invoke non-portable SQL time functions (as used currently). The "os" module is not included in the restricted environment.

In general, it seems a bit daft for the database script, which has read/write access to some of the highest-value data the server deals with (user credentials) as well as a suite of general-purpose database access functions, to run with the same restrictions as "scenario" scripts. Perhaps this instance should run in an unrestricted Lua?

The threat model which the Lua security measures are responding to is a bit murky. Bug #15624 indicates that the desire is to allow users to continue to confidently treat downloaded scenarios/rulesets as "simple data", without any risk that embedded scripts can have any nasty effects beyond the game environment they're running in (such as disclosing or overwriting user data outside Freeciv). Now we have freeciv-modpack, that assurance should probably extend to anything a user downloads with it.

It seems fairly clear that that threat model shouldn't apply to database.lua, because that's essentially part of the server. So it's tempting to remove the restrictions for the auth instance. (Perhaps that would also mollify Michal per bug #19006?)

However...

database.lua is loaded from the data path, which defaults to:

and can be overridden by environment variables FREECIV_DATA_PATH and FREECIV_PATH. Thus, there is plenty of scope for an attacker to override the standard database.lua by tricking a user into running the server from a chosen directory, or (I think) by tricking them into downloading a dodgy modpack via freeciv-modpack.

Even with things as they stand, such an attacker would be able to impersonate other Freeciv server users, or possibly leak their credentials via side channels. If we remove the Lua restrictions for database.lua, they could do much more.

Looking at the contents of data/, I think database.lua is probably the first file in there with this sort of security property. Other "code"-y files include game scripts (handled by existing Lua security) and .serv files (only limited potential for mischief -- "save ~/.bashrc").

So, even if I find another solution for my os.time() issue (such as whitelisting bits of os.*), we still have a problem. What should we do about it?

  • Invent a new category of "data" file, with database.lua as the only current member, loaded from a much more restrictive default path that only includes the installation path (cf patch #2827)?
    • How will a non-root server operator override it on a machine where Freeciv is installed system-wide? Is an environment variable acceptable? That would still be sufficient guard against the dodgy-modpack / chosen-directory attack.

If we do mitigate the path problem, would there be any other issue with relaxing the restrictions on database.lua?

Jacob Nevins <jtn>
Project Administrator

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #16014:  trunk-dbluaLoc-bis.patch added by jtn (3kB - text/x-diff - trunk/S2_4 r21469: compiles & runs)
file #16015:  S2_4-dbluaLoc-bis.patch added by jtn (4kB - text/x-diff - trunk/S2_4 r21469: compiles & runs)
file #15976:  dbluaLoc.diff added by cazfi (2kB - text/x-diff)

 

Digest:
   bug dependencies.

Digest:
   patch dependencies.

 

Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by jtn (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 13 Aug 2014 01:34:53 AM UTCcazfiPlanned Release2.5.0=>2.6.0
    Sun 08 Jul 2012 05:23:34 PM UTCjtnPlanned Release2.4.0,2.5.0=>2.5.0
      Dependencies-=>Depends on bugs #19911
    Sun 08 Jul 2012 12:07:41 AM UTCjtnAttached File-=>Added trunk-dbluaLoc-bis.patch, #16014
      Attached File-=>Added S2_4-dbluaLoc-bis.patch, #16015
    Thu 05 Jul 2012 03:17:04 AM UTCcazfiAttached File-=>Added dbluaLoc.diff, #15976
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup