Malicious code exploit possible in STB based file formats...

This message should not cause much alarm however it should serve as a
heads-up about possible security issues related to deployed Dolphin
applications that make use of STB as a document file format.  This may be an
obvious issue, but could be easily overlooked.

The Dolphin STB binary persistence mechanism can store blocks of code.
SortedCollections save their sort blocks when they are persisted to STB.
The sort block is re-evaluated when the SortedCollection is reconstituted
from STB.  Unless an application provides a fair amount of validation before
reconstituting STB files a devious person could simply persist a two element
SortedCollection with a malicious code block.  The code block would
automatically run when the STB file was loaded.  Hence a document file
format that an end user may have no reason to believe could contain
executable code now does.  As one can imagine there are a number of bad
things that can be done once someone can inject code into a system without
the knowledge of the user.

This is a brief example of the issue:
=========================
"Create the evil object."
sc := SortedCollection
    sortBlock: [:itemA :itemB |
        MessageBox warning: 'Potentially evil code runs here!' caption:
'Boo!'.
        false].
sc add: nil; add: nil.
"Now persist and reconstitute the evil object."
Object fromBinaryStoreBytes: sc binaryStoreBytes.
=========================

I don't consider this a problem with the STB implementation, this is a
feature by design.  STB is just another tool, and like any tool it gives us
enough rope to hang ourselves if we are not careful.

So... Any thoughts on how to improve the security of STB documents in
deployed applications?  I am thinking an encryption or signing technique
could be used to verify the file.  These protections could possibly be
circumvented, but at least it makes it a good deal harder to use this
exploit.  I suppose another option might be to add a way to prohibit loading
executable code from STB files, and then manually reset the sort blocks
etc... as needed (this may be a good approach for other reasons).  So what
are others doing about this?

Chris

Chris,

I rather dramatic subject but you are correct that the issue exists. Why not
use DolphinSure to sign the documents?

Best Regards,

Andy Bower
Dolphin Support
http://www.object-arts.com
---
Are you trying too hard?
http://www.object-arts.com/Relax.htm
---

"Christopher J. Demers" <[hidden email]> wrote in
message news:appvif$41doa$[hidden email]...
> This message should not cause much alarm however it should serve as a
> heads-up about possible security issues related to deployed Dolphin
> applications that make use of STB as a document file format.  This may be
an
> obvious issue, but could be easily overlooked.
>
> The Dolphin STB binary persistence mechanism can store blocks of code.
> SortedCollections save their sort blocks when they are persisted to STB.
> The sort block is re-evaluated when the SortedCollection is reconstituted
> from STB.  Unless an application provides a fair amount of validation
before
> reconstituting STB files a devious person could simply persist a two
element us
> enough rope to hang ourselves if we are not careful.
>
> So... Any thoughts on how to improve the security of STB documents in
> deployed applications?  I am thinking an encryption or signing technique
> could be used to verify the file.  These protections could possibly be
> circumvented, but at least it makes it a good deal harder to use this
> exploit.  I suppose another option might be to add a way to prohibit
loading
> executable code from STB files, and then manually reset the sort blocks
> etc... as needed (this may be a good approach for other reasons).  So what
> are others doing about this?
>
> Chris
>
>

Christopher J. Demers wrote:

>  Hence a document file
> format that an end user may have no reason to believe could contain
> executable code now does.

I have tended to be a bit leery of the STB for exactly that reason.  The
same reasoning applies (perhaps even more so) when using STB-over-the-Net as
in the Chat example program (at least I think it was Chat that did it, it's
a long time since I last looked).

I don't think there can be a good general purpose solution.  Encrypting it
or signing it will help, but the problem is that the decryption *and*
encryption keys (or public *and* private) keys have to be embedded in the
application, so there's rather a weak spot there.

If the application is such that you could have a secure central server, then
it could act to distribute one-time keys for each saved document.  The app
would ask the server for a key for a given user/filename pair, and the
server would generate and answer one.  When the app wanted to re-open the
file, it would go back to the server for the unlock key.  Rather a
heavyweight solution, though!

I suppose you could use the user in place of the central server.  I.e. make
them supply a password when they saved, and supply it again to re-open the
file.  That might be unpopular, unless the stored data was something that
the users would want to encrypt and password-protect anyway.

Another idea would be a validating STB scanner which would scan an STBed
document and answer a set of all the classes of objects contained therein.
You would then check that list to see if it looked OK.  It'd be a bit more
complicated than it sounds because it'd have to take account of objects that
are STBed as proxies (but without resuscitating them).  Probably quite
doable, though.  And it'd only have to be written once.

Another similar approach (that I've only just thought of this minute) would
be to provide the STBInFiler with a modified ClassLocator that threw
a resumable exception on every classname before it resolved it (from
#findResidentClass, I suppose).  That the app would catch those exceptions,
and -- if it was happy with the classname -- resume the processing, if not
then it would abort the operation.  Of course, you could also use a custom
ClassLocator that knew how to do the checking itself, but that's not so
general.

Or, taking that further (I'm thinking aloud here, you can tell), it'd be
possible to create a CheckingSTBInFiler that took pluggable validation
blocks for each class and object.  Might be a useful addition to the
standard image.

    -- chris

Andy Bower <[hidden email]> wrote in message
news:apqv8l$45ami$[hidden email]...
> Chris,
>
> I rather dramatic subject but you are correct that the issue exists. Why
not
> use DolphinSure to sign the documents?

I apologize if the subject sounds overly dramatic. However it might be worth
adding a warning about this possibility to the STB documentation.

I think there would be some problems with using DolphinSure for my files.

First I think I have to get an official certificate from OA, then
essentially the certificate private and public keys would have to be
included in the deployed image.  I don't think I would want to do this if it
were a real company certificate, so I guess I could get a "file format"
certificate, but I am not sure if that fits in the scheme of what
DolphinSure certificates were intended for.  If I could just make my own
unofficial certificate that might work, but I am not sure if that can be
done.

The next problem appears to be that DolphinSure requires that the
DolphinSureTrustedData object be stored in STB itself.  I don't see a
built-in way to ensure that an object is of an expected class when
reconstituting objects from an STB file.  So STB instantiates objects and
could run code in blocks before I ever get a chance to validate the
DolphinSureTrustedData.  To have real security value I would need a way of
prefixing files with a non-executable signature that could be used to
validate the rest of the potentially executable data in the file.

I am going to send you an e-mail regarding some additional things I have
just noticed.  Contact me if you do not get it.

Chris

Chris, Andy,

How about this: perhaps by using a subclass of the in-filer (so there's no
overhead in the majority of case where it is not an issue), add a way to
complain based on the classes present in the stream.  I have a situation in
which I really want to know if I'm saving blocks, mostly in development so I
can re-think the "mistake" (or convince myself that it really is justified).
The same trick that would allow me to see that a block slipped into my
stream might allow Chris to validate his data.  Chris, would that suffice
for you?

Comments??

Have a good one,

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]

Chris Uppal <[hidden email]> wrote in message
news:[hidden email]...
>
> I don't think there can be a good general purpose solution.  Encrypting it
> or signing it will help, but the problem is that the decryption *and*
> encryption keys (or public *and* private) keys have to be embedded in the
> application, so there's rather a weak spot there.

I have been thinking about this.  It would provide some security.  Maybe...

> Another idea would be a validating STB scanner which would scan an STBed
> document and answer a set of all the classes of objects contained therein.
> You would then check that list to see if it looked OK.  It'd be a bit more
> complicated than it sounds because it'd have to take account of objects
that
...
> Or, taking that further (I'm thinking aloud here, you can tell), it'd be
> possible to create a CheckingSTBInFiler that took pluggable validation
> blocks for each class and object.  Might be a useful addition to the
> standard image.

I _think_ all I want to do is prohibit blocks and Messages from being read
from STB files.  From a security perspective I don't think I care if objects
different from what I am expecting are created, so long as no code can be
run.  I think only blocks and Messages can cause code to run.  I might make
a SafeSTBInFiler and raise an exception if it sees a block or Message.  This
will make my life a little harder where I happen to be saving
SortedCollections.  I will need to clear the sort block, and reset it when
it is loaded.  This may be a good idea in general,  otherwise objects loaded
from old files could be using an obsolete sort block if I changed it in the
code.

I just can't think of any safe way to permit blocks to be saved and read
from an STB file without signing or (prohibitively) intense scanning.  I
think I just have to prohibit blocks and work around them.

Thank you for your comments.

Chris

Bill Schwab <[hidden email]> wrote in message
news:apsjn8$4m9hb$[hidden email]...
>
> How about this: perhaps by using a subclass of the in-filer (so there's no
> overhead in the majority of case where it is not an issue), add a way to
> complain based on the classes present in the stream.  I have a situation
in
> which I really want to know if I'm saving blocks, mostly in development so
I
> can re-think the "mistake" (or convince myself that it really is
justified).
> The same trick that would allow me to see that a block slipped into my
> stream might allow Chris to validate his data.  Chris, would that suffice
> for you?

I just made one.  Interested parties can have a look here
http://www.mitchellscientific.com/smalltalk/SafeSTBInFiler.cls .

I think this approach will satisfy my immediate security concerns regarding
my own use of STB as a document file format.

This class will prohibit BlockClosure or Message objects from being read
because they are unsafe since they could cause foreign code to execute in
the image.  This class can be used in place of STBInFiler in cases where an
untrusted document file needs to be opened without the risk of it evaluating
code.  Objects that can contain blocks (SortedCollections etc...) will need
to be stored without their blocks and have them manually recreated after
they are loaded if one intends to load them via this class.

Is prohibiting BlockClosure and Message enough, or are there other classes
that could cause external code to be run?

Perhaps I should have called the class SaferSTBInFiler, as nothing is truly
safe in this world. ;)

Chris

Christopher J. Demers wrote:

> I _think_ all I want to do is prohibit blocks and Messages from being read
> from STB files.  From a security perspective I don't think I care if
objects
> different from what I am expecting are created, so long as no code can be
> run.  I think only blocks and Messages can cause code to run.

You may well be right for your specific application, but in general I'd
advise looking at it the other way around: *any* class *could* be malicious,
and unless you want to check them all on a case-by-case basis, then you are
probably better off refusing any class that you don't "know" about.  Two
examples:

================
Object subclass: 'MaliciousSortBlock' [other parameters snipped].

MaliciousSortBlock class>>value:value:
    "payload goes here"

sc := (SortedCollection new: MaliciousSortBlock) add: nil; add: nil;
yourself.
bytes := sc binaryStoreBytes.
================

and:

================
f := File open: 'C:\WinNT\SomethingCritical' mode: #truncate check: false.
bytes := f binaryStoreBytes.
================

Now, the first of these examples is not going to be a problem for a stripped
app (and probably not for much else either), but it illustrates a point that
it's the #value:value: method you have to be wary of.  But there may be
other methods to be wary of, if someone can think of other objects than
SortBlock that automatically send #value-style messages to other objects.
(As an example, I haven't checked that it can work, but how about
CompiledMethods ?)

And the second example doesn't work at all!  But the point I'm trying to
illustrate is that the only way you can know that is by having tried it (or
taking someone's word for it ;-).  How many other similar cases are there in
the image that would need checking ?  I don't know and I don't want to have
to find out by slogging through 30-odd K methods...  (BTW, I think Blair
once posted an example using MessageBox.)

    -- chris

Chris,

> I just made one.  Interested parties can have a look here
> http://www.mitchellscientific.com/smalltalk/SafeSTBInFiler.cls .
>
> I think this approach will satisfy my immediate security concerns
regarding
> my own use of STB as a document file format.
>
> This class will prohibit BlockClosure or Message objects from being read
> because they are unsafe since they could cause foreign code to execute in
> the image.  This class can be used in place of STBInFiler in cases where
an
> untrusted document file needs to be opened without the risk of it
evaluating
> code.  Objects that can contain blocks (SortedCollections etc...) will
need
> to be stored without their blocks and have them manually recreated after
> they are loaded if one intends to load them via this class.

It seems to me that there are two interrelated issues here. The first is
that (as it stands) it is possible to create an STB fileout that will
execute malicious code *as the file is being loaded*. This is the case with
your SortedCollection example. The second issue is that, once an STB object
has been loaded then it may contain new code either in the form of a
BlockClosure or a new class that may be malicious and executed when a
message is sent to the STB object.

1) The only imported code that gets executed during the loading process (as
far as I'm aware) is that contained in STBProxy objects that have been
stored in the stream. The proxies are there to cover unusual situtations
where something needs to be done to fix up the loaded object. In the case of
SortedCollections it is because the object hashes will no longer be valid.
As you'll see there are a limited number of STBProxy subclasses and I
"think" the only one that needs to be addressed is STBSortedCollectionProxy.
This is the only one that stores a block to be executed on import. If we can
come up with a better proxy that doesn't rebuild the sorted collection in
this way (see SortedCollectionProxy>>value) this might address this issue.

2) So with 1) fixed we'd be able to "load" STB without a problem including
files that contained sorted collections. However, as soon as any message is
sent to the loaded object there is a potential security issue that comes
about because the STB may have loaded new classes/blocks/messagesends. I
think if your SafeSTBInFiler were modified to also deny new classes this
would be addressed.

> Is prohibiting BlockClosure and Message enough, or are there other classes
> that could cause external code to be run?

I'm not sure/. We'd have to think about that.

Best Regards,

Andy Bower
Dolphin Support
http://www.object-arts.com
---
Are you trying too hard?
http://www.object-arts.com/Relax.htm
---

Andy Bower <[hidden email]> wrote in message
news:aptodt$4rdse$[hidden email]...
>
> 1) The only imported code that gets executed during the loading process
(as
> far as I'm aware) is that contained in STBProxy objects that have been
> stored in the stream. The proxies are there to cover unusual situtations
> where something needs to be done to fix up the loaded object. In the case
of
> SortedCollections it is because the object hashes will no longer be valid.
> As you'll see there are a limited number of STBProxy subclasses and I
> "think" the only one that needs to be addressed is
STBSortedCollectionProxy.
> This is the only one that stores a block to be executed on import. If we
can
> come up with a better proxy that doesn't rebuild the sorted collection in
> this way (see SortedCollectionProxy>>value) this might address this issue.

I am not sure if it is bad that the SortedCollection is resorted after
reincarnation from an STB form.  When I first encountered this issue my
first reaction was to wonder why the SortedCollection had to resort it self
if it was saved in the sort order.  However it occurred to me that maybe the
code responsible for the sort (not directly in the block, but in methods the
block code uses) could change.  For example if a SortedCollection were saved
in version 1.0 of MyProgram and then opened in version 2.0 of MyProgram, but
that version had different code in the #< method the sort order would not be
consistent with the new code in the 2.0 image.  Now if elements were added
the collection would be strange because some elements would be sorted the
old way, and some the new way.  I am not sure how critical this is, but it
worth considering the implications.

> 2) So with 1) fixed we'd be able to "load" STB without a problem including
> files that contained sorted collections. However, as soon as any message
is
> sent to the loaded object there is a potential security issue that comes
> about because the STB may have loaded new classes/blocks/messagesends. I
> think if your SafeSTBInFiler were modified to also deny new classes this
> would be addressed.

Good idea, I did not think of new classes.

> > Is prohibiting BlockClosure and Message enough, or are there other
classes
> > that could cause external code to be run?
>
> I'm not sure/. We'd have to think about that.

If people extend those classes with subclasses they could be used to get
around the restrictions.  Of course the bad guy would have to know the names
of those classes in advance of the attack.  I am not sure if it is worth
changing the checks to isKindOf: .  If there is not much speed difference
between == and isKindOf: then isKindOf: might be better.

Chris

Chris Uppal <[hidden email]> wrote in message
news:[hidden email]...
> Christopher J. Demers wrote:
>
> > I _think_ all I want to do is prohibit blocks and Messages from being
read
> > from STB files.  From a security perspective I don't think I care if
> objects
> > different from what I am expecting are created, so long as no code can
be
> > run.  I think only blocks and Messages can cause code to run.
>
> You may well be right for your specific application, but in general I'd
> advise looking at it the other way around: *any* class *could* be
malicious,
> and unless you want to check them all on a case-by-case basis, then you
are
> probably better off refusing any class that you don't "know" about.  Two
> examples:
...

You have a good point.  My only concern would be keeping the list of
expected classes updated and testing all output scenarios.  I guess this
approach would force me to keep track of what exactly is being saved in my
STB files (which is not a bad ideas anyway).  I suppose I could write some
code to generate a collection of used classes based on an STB file.  Then I
could pass this list of expected classes to my SafeSTBFiler it could be
optional depending upon the level of protection required.

Chris