Skip to content

Commit

Permalink
tresor: check hash of all read vba data
Browse files Browse the repository at this point in the history
During one of the many re-factorization steps that were applied to the Tresor
library and its predecessor, the CBE library, one of the main features of the
project, the integrity check, accidentally received a grave regression. The
most recent version of the Tresor still used to check all hashes of meta-data
blocks but ignored the hashes of the actual data blocks.

With this commit, the hashes of all data-block get checked. Note, I have
included also the hashes of yet uninitialized data blocks although they were,
up until now, always ignored on purpose. The reason for ignoring uninitialized
blocks was that they are not actually read from disc but simply generated as
an all-zeros block in the driver in order to prevent having to initialize them
all to zero in Tresor-Init. Therefore, the integrity of these blocks cannot be
compomised and the stored hashes of these blocks are guarded by the above hash
tree.

I decided to check these hashes anyway for two reasons. First, it simplifies
the code and makes it easier to verify that integrity is indeed preserved. And
second, not checking the hashes would allow for Tresor containers that are
broken in a certain way to be still accepted by the driver, thereby potentially
covering up regressions in the Tresor tooling.

This commit also adapts the Tresor initializer and check modules to the new
behavior regarding hashes of uninitialized data blocks.

Ref genodelabs#5062
  • Loading branch information
m-stein committed Nov 30, 2023
1 parent a6fd2bd commit a3cf11d
Show file tree
Hide file tree
Showing 9 changed files with 36 additions and 19 deletions.
1 change: 1 addition & 0 deletions repos/gems/run/tresor_tester.run
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ append config {

<request op="write" vba="1" count="1" sync="yes" salt="1234"/>
<request op="read" vba="1" count="1" sync="yes" salt="1234"/>
<request op="read" vba="2" count="2" sync="yes"/>
<request op="write" vba="12" count="10" sync="yes"/>
<request op="read" vba="12" count="10" sync="yes"/>
<request op="write" vba="24" count="40" sync="yes"/>
Expand Down
1 change: 1 addition & 0 deletions repos/gems/src/lib/tresor/block_io.cc
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ void Block_io_channel::_read_client_data(bool &progress)
case REQ_SUBMITTED: _file.read(READ_OK, FILE_ERR, req._pba * BLOCK_SIZE, { (char *)&_blk, BLOCK_SIZE }, progress); break;
case READ_OK:

calc_hash(_blk, req._hash);
_generate_req<Crypto_request>(
PLAINTEXT_BLK_SUPPLIED, progress, Crypto_request::DECRYPT_CLIENT_DATA, req._client_req_offset,
req._client_req_tag, req._key_id, *(Key_value *)0, req._pba, req._vba, _blk);
Expand Down
4 changes: 2 additions & 2 deletions repos/gems/src/lib/tresor/include/tresor/block_io.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ class Tresor::Block_io : public Module
struct Read_client_data : Request
{
Read_client_data(Module_id m, Module_channel_id c, Physical_block_address p, Virtual_block_address v,
Key_id k, Request_tag t, Request_offset o, bool &s)
: Request(m, c, Request::READ_CLIENT_DATA, o, t, k, p, v, *(Block*)0, *(Hash*)0, s) { }
Key_id k, Request_tag t, Request_offset o, Hash &h, bool &s)
: Request(m, c, Request::READ_CLIENT_DATA, o, t, k, p, v, *(Block*)0, h, s) { }
};

Block_io(Vfs::Env &, Xml_node const &);
Expand Down
7 changes: 3 additions & 4 deletions repos/gems/src/lib/tresor/include/tresor/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -649,10 +649,9 @@ struct Tresor::Snapshot
Genode::print(out, "<invalid>");
}

bool contains_vba(Virtual_block_address vba) const
{
return vba <= nr_of_leaves - 1;
}
bool contains_vba(Virtual_block_address vba) const { return vba <= nr_of_leaves - 1; }

Type_1_node t1_node() const { return { pba, gen, hash }; }
};


Expand Down
1 change: 1 addition & 0 deletions repos/gems/src/lib/tresor/include/tresor/vbd_initializer.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class Tresor::Vbd_initializer_channel : public Module_channel
Node_state _node_states[TREE_MAX_NR_OF_LEVELS][NUM_NODES_PER_BLK] { DONE };
bool _generated_req_success { false };
Block _blk { };
Hash _leaf_hash { };
Number_of_leaves _num_remaining_leaves { };

NONCOPYABLE(Vbd_initializer_channel);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class Tresor::Virtual_block_device_channel : public Module_channel

void _read_vba(bool &);

bool _check_and_decode_read_blk(bool &, bool);
bool _check_and_decode_read_blk(bool &);

void _mark_req_successful(bool &);

Expand Down
11 changes: 10 additions & 1 deletion repos/gems/src/lib/tresor/vbd_check.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,20 @@ bool Vbd_check_channel::_execute_node(Tree_level_index lvl, Tree_node_index node
break;
}
if (node.gen == INITIAL_GENERATION) {
if (VERBOSE_CHECK)
log(Level_indent { lvl, req._vbd.max_lvl }, " lvl ", lvl, " node ", node_idx, " (", node,
"): initial gen, assume lvl ", lvl - 1, " to be 0");

memset(&_blk, 0, BLOCK_SIZE);
if (!check_hash(_blk, node.hash)) {
_mark_req_failed(progress, { "lvl ", lvl, " node ", node_idx, " (", node, ") has bad hash" });
break;
}
_num_remaining_leaves--;
check_node = false;
progress = true;
if (VERBOSE_CHECK)
log(Level_indent { lvl, req._vbd.max_lvl }, " lvl ", lvl, " node ", node_idx, ": uninitialized");
log(Level_indent { lvl, req._vbd.max_lvl }, " lvl ", lvl, " node ", node_idx, ": good hash");
break;
}
} else {
Expand Down
4 changes: 3 additions & 1 deletion repos/gems/src/lib/tresor/vbd_initializer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ bool Vbd_initializer_channel::_execute_node(Tree_level_index lvl, Tree_node_inde

if (lvl == 1)
if (_num_remaining_leaves) {
node = { };
node = { 0, INITIAL_GENERATION, _leaf_hash };
if (!_req_ptr->_pba_alloc.alloc(node.pba)) {
_mark_req_failed(progress, "allocate pba");
break;
Expand Down Expand Up @@ -152,6 +152,8 @@ void Vbd_initializer_channel::execute(bool &progress)
switch (_state) {
case SUBMITTED:

memset(&_blk, 0, BLOCK_SIZE);
calc_hash(_blk, _leaf_hash);
_num_remaining_leaves = req._vbd.num_leaves;
for (Tree_level_index lvl = 0; lvl < TREE_MAX_LEVEL; lvl++)
_reset_level(lvl, Vbd_initializer_channel::DONE);
Expand Down
24 changes: 14 additions & 10 deletions repos/gems/src/lib/tresor/virtual_block_device.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ void Virtual_block_device_channel::_read_vba(bool &progress)

case READ_BLK_SUCCEEDED:
{
if (!_check_and_decode_read_blk(progress, false))
if (!_check_and_decode_read_blk(progress))
break;

if (!_lvl) {
Expand All @@ -113,7 +113,7 @@ void Virtual_block_device_channel::_read_vba(bool &progress)
} else
_generate_req<Block_io::Read_client_data>(
READ_BLK_SUCCEEDED, progress, node.pba, _vba, req._curr_key_id,
req._client_req_tag, req._client_req_offset);
req._client_req_tag, req._client_req_offset, _hash);
_lvl--;
break;
}
Expand Down Expand Up @@ -150,16 +150,20 @@ void Virtual_block_device_channel::_update_nodes_of_branch_of_written_vba()
}


bool Virtual_block_device_channel::_check_and_decode_read_blk(bool &progress, bool check_leaf_data = true)
bool Virtual_block_device_channel::_check_and_decode_read_blk(bool &progress)
{
if (!check_leaf_data && !_lvl)
Type_1_node node { _lvl < snap().max_level ?
_t1_blks.items[_lvl + 1].nodes[t1_node_idx_for_vba(_vba, _lvl + 1, _req_ptr->_snap_degr)] :
snap().t1_node() };

if (_req_ptr->_type == Request::READ_VBA && !_lvl && node.gen != INITIAL_GENERATION) {
if (_hash != node.hash) {
_mark_req_failed(progress, "check hash of read block");
return false;
}
return true;

Hash &hash { _lvl < snap().max_level ?
_t1_blks.items[_lvl + 1].nodes[t1_node_idx_for_vba(_vba, _lvl + 1, _req_ptr->_snap_degr)].hash :
snap().hash };

if (!check_hash(_lvl ? _encoded_blk : _data_blk, hash)) {
}
if (!check_hash(_lvl ? _encoded_blk : _data_blk, node.hash)) {
_mark_req_failed(progress, "check hash of read block");
return false;
}
Expand Down

0 comments on commit a3cf11d

Please sign in to comment.