| Age | Commit message (Collapse) | Author |
|---|
|
APT in 1.6 saw me rewriting the mirror:// transport method, which works
comparable to the decommissioned httpredir.d.o "just" that apt requests
a mirror list and performs all the redirections internally with all the
bells like parallel download and automatic fallback (more details in the
apt-transport-mirror manpage included in the 1.6 release).
The automatic fallback is the problem here: The intend is that if a file
fails to be downloaded (e.g. because the mirror is offline, broken,
out-of-sync, …) instead of erroring out the next mirror in the list is
contacted for a retry of the download.
Internally the acquire process of an InRelease file (works with the
Release/Release.gpg pair, too) happens in steps: 1) download file and 2)
verify file, both handled as URL requests passed around. Due to an
oversight the fallbacks for the first step are still active for the
second step, so that the successful download from another mirror stands
in for the failed verification… *facepalm*
Note that the attacker can not judge by the request arriving for the
InRelease file if the user is using the mirror method or not. If entire
traffic is observed Eve might be able to observe the request for
a mirror list, but that might or might not be telling if following
requests for InRelease files will be based on that list or for another
sources.list entry not using mirror (Users have also the option to have
the mirror list locally (via e.g. mirror+file://) instead of on a remote
host). If the user isn't using mirror:// for this InRelease file apt
will fail very visibly as intended.
(The mirror list needs to include at least two mirrors and to work
reliably the attacker needs to be able to MITM all mirrors in the list.
For remotely accessed mirror lists this is no limitation as the attacker
is in full control of the file in that case)
Fixed by clearing the alternatives after a step completes (and moving a pimpl
class further to the top to make that valid compilable code). mirror://
is at the moment the only method using this code infrastructure (for all
others this set is already empty) and the only method-independent user
so far is the download of deb files, but those are downloaded and
verified in a single step; so there shouldn't be much opportunity for
regression here even through a central code area is changed.
Upgrade instructions: Given all apt-based frontends are affected, even
additional restrictions like signed-by are bypassed and the attack in
progress is hardly visible in the progress reporting of an update
operation (the InRelease file is marked "Ign", but no fallback to
"Release/Release.gpg" is happening) and leaves no trace (expect files
downloaded from the attackers repository of course) the best course of
action might be to change the sources.list to not use the mirror family
of transports ({tor+,…}mirror{,+{http{,s},file,…}}) until a fixed
version of the src:apt packages are installed.
Regression-Of: 355e1aceac1dd05c4c7daf3420b09bd860fd169d,
57fa854e4cdb060e87ca265abd5a83364f9fa681
LP: #1787752
|
|
Check that Date of Release file is not in the future
See merge request apt-team/apt!3
|
|
By restricting the Date field to be in the past, an attacker cannot
just create a repository from the future that would be accepted as
a valid update for a repository.
This check can be disabled by Acquire::Check-Date set to false. This
will also disable Check-Valid-Until and any future date related checking,
if any - the option means: "my computers date cannot be trusted."
Modify the tests to allow repositories to be up to 10 hours in the
future, so we can keep using hours there to simulate time changes.
|
|
The interesting takeaway here is perhaps that 'chmod +w' is effected by
the umask – obvious in hindsight of course. The usual setup helps with
hiding that applying that recursively on all directories (and files)
isn't correct. Ensuring files will not be stored with the wrong
permissions even if in strange umask contexts is trivial in comparison.
Fixing the test also highlighted that it wasn't bulletproof as apt will
automatically fix the permissions of the directories it works with, so
for this test we actually need to introduce a shortcut in the code.
Reported-By: Ubuntu autopkgtest CI
|
|
apt 1.6~alpha6 introduced aux requests to revamp the implementation of
a-t-mirror. This already included the potential of running as non-root,
but the detection wasn't complete resulting in errors or could produce
spurious warnings along the way if the directory didn't exist yet.
References: ef9677831f62a1554a888ebc7b162517d7881116
Closes: 887624
|
|
Allow specifying an alternative path to the InRelease file, so
you can have multiple versions of a repository, for example.
Enabling this option disables fallback to Release and Release.gpg,
so setting it to InRelease can be used to ensure that only that
will be tried.
We add two test cases: One for checking that it works, and another
for checking that the fallback does not happen.
Closes: #886745
|
|
If a method needs a file to operate like e.g. mirror needs to get a list
of mirrors before it can redirect the the actual requests to them. That
could easily be solved by moving the logic into libapt directly, but by
allowing a method to request other methods to do something we can keep
this logic contained in the method and allow e.g. also methods which
perform binary patching or similar things.
Previously they would need to implement their own acquire system inside
the existing one which in all likelyhood will not support the same
features and methods nor operate with similar security compared to what
we have already running 'above' the requesting method. That said, to
avoid methods producing conflicts with "proper" files we are downloading
a new directory is introduced to keep the auxiliary files in.
[The message magic number 351 is a tribute to the german Grundgesetz
article 35 paragraph 1 which defines that all authorities of the
state(s) help each other on request.]
|
|
Earlier gcc versions used to complain that you should add them althrough
there isn't a lot of point to it if you think about it, but now gcc (>= 8)
complains about the attribute being present.
warning: ‘pure’ attribute on function returning ‘void’ [-Wattributes]
Reported-By: gcc -Wattributes
Gbp-Dch: Ignore
|
|
For deb files we always supported falling back from one server to the
other if one failed to download the deb, but that was hardwired in the
handling of this specific item. Moving this alongside the retry
infrastructure we can implement it for all items and allow methods to
use this as well by providing additional URIs in a redirect.
|
|
We have quite a bit of metadata available for the files we acquire, but
the methods weren't told about it and got just the URI. That is indeed
fine for most, but to avoid methods trying to parse the metadata out of
the provided URIs (and fail horribly in edgecases) we can just as well
be nice and tell them stuff directly.
|
|
Moving the Retry-implementation from individual items to the worker
implementation not only gives every file retry capability instead of
just a selected few but also avoids needing to implement it in each item
(incorrectly).
|
|
gcc has problems understanding this construct and additionally thinks it
would produce multiple lines and stuff, so to keep using it isn't really
worth it for the few instances we have: We can just write the long form
there which works better.
Reported-By: gcc
Gbp-Dch: Ignore
|
|
If a InRelease file fails to download with a non-404 error
we assumed there is some general problem with repository like
a webportal or your are blocked from access (wrong auth, Tor, …).
Turns out some server like S3 return 403 if a file doesn't exist.
Allowing this in general seems like a step backwards as 403 is a
reasonable response if auth failed, so failing here seems better
than letting those users run into problems.
What we can do is show our insecure warnings through and allow the
failures for insecure repos: If the repo is signed it is easy to add
an InRelease file and if not you are setup for trouble anyhow.
References: cbbf185c3c55effe47f218a07e7b1f324973a8a6
|
|
Regression-Of: cc1f94c95373670fdfdb8e2d6cf9125181f7df0c
|
|
As a follow up to the last commit, let's replace APT_CONST
with APT_PURE everywhere to clean stuff up.
|
|
The comment says this is intended, but looking at the history reveals
that the comment comes from a different era. Nowadays we don't really
need it anymore (and even back then it was disputeable) as we haven't
used that file for our update in the end and nothing really needs this
file after the update.
Triggered is this by 188f297a2af4c15cb1d502360d1e478644b5b810 which
moves various error conditions forward including this code expecting the
file to exist – but it doesn't need to as download could have failed.
We could fix that by simple checking if the file exists and only stage
it if it does, but instead we don't stage it and instead even rename it
out of the way with our conventional FAILED name (if it exists).
That restores support for partial mirrors (= in this case mirrors which
don't ship pdiff files). Note that apt heals itself even if only such a
mirror is used as the update is successful even if that error is shown.
Closes: 869425
|
|
RenameOnError does the rename already, so the check for existence will
always fail making this some completely harmles but also completely
pointless two lines of code we are better of removing.
Gbp-Dch: Ignore
|
|
Minor grammar fix
[jak@d.o: Fixed up po/]
|
|
This makes it easier to see which headers includes what.
The changes were done by running
git grep -l '#\s*include' \
| grep -E '.(cc|h)$' \
| xargs sed -i -E 's/(^\s*)#(\s*)include/\1#\2 include/'
To modify all include lines by adding a space, and then running
./git-clang-format.sh.
|
|
Including cacheiterators.h before pkgcache.h fails because
pkgcache.h depends on cacheiterators.h.
|
|
Progress only shows if we have an idea of how much files we will
acquire, but if a transaction fails before we have got an idea we ended
up never showing progress even through we know that a failed transaction
will not download additional files.
|
|
Having messages being printed on the error stack and confirm them by
commandline flags is an okayish first step, but some frontends will
probably want to have a more interactive feeling here with a proper
question the user can just press yes/no for as for some frontends a
commandline flag makes no sense…
|
|
This gives the repository owner a chance to explain why this change was
needed – e.g. explaining the organisational changes or simply detailing
the changes in the new release made. Note that this URI is also shown
if the change is accepted, so it also draws attention to release notes
of minor updates (if users watch apt output closely).
|
|
The value of Origin, Label, Codename and co can be used in user
configuration from apts own pinning to unattended upgrades.
A repository changing this values can therefore have serious effects on
the behaviour of apt and other tools using these values.
In a first step we will generate error messages for these changes now
explaining the need for explicit confirmation and provide config options
and commandline flags to accept them.
|
|
There are very many HTTP errorcodes which indicate that the repository
isn't available at the moment or the connection has some kind of
problem. Given that we do not require Release files the result was that
these errors were ignored and the user presented with a message like
"Repository is no longer signed" which sends the user in the wrong
direction.
Instead of trying to figure out which http errorcodes indicate a global
problem we accept only 404 for ignoring and consider all the rest as
hard errors now causing us to stop instantly after the InRelease file
and print the errorcode (with short description from server) received.
|
|
Moving the code responsible for parsing the Index file from ::Done into
the slightly earlier ::VerifyDone allows us to still "fail" the download
if we can't make use of the Index for whatever reason, so that the
progress log correctly displays "Ign" instead of "Get" for the file.
This also makes quiet a few debug messages proper error messages (but
those are still hidden by default for Ign lines).
|
|
If we couldn't find an entry for a Sources file we would generate an
error while for a Packages file we would silently skip it due to
assuming it is missing because it is empty. We can do better by checking
if the repository declares that it supports a component we want to get
the file from and if not say so and hint at the user making a typo.
An example were this helps is mozilla.debian.net which dropped the
firefox-aurora component (as upstream did) meaning no upgrades until the
user notices manually that the repository doesn't provide packages
anymore. With this commit warnings are raised hopefully causing the user
to investigate what is wrong (sooner).
|
|
Modified the wording of an error message when a repository no longer has a release file.
|
|
It says SRCNAME_SRCVER, but the example just gives
the SRCVER part.
Reported-By: Nishanth Aravamudan (nacc) in #ubuntu-devel
|
|
Most of them in (old) code comments. The two instances of user visible
string changes the po files of the manpages are fixed up as well.
Gbp-Dch: Ignore
Reported-By: spellintian
|
|
In ad9416611ab83f7799f2dcb4bf7f3ef30e9fe6f8 we fall back to asking the
original mirror (e.g. a redirector) if we do not get the expected
result. This works for the indexes, but patches are a different beast
and much simpler. Adding this fallback code here seems like overkill as
they are usually right along their Index file, so actually forward the
relevant settings to the patch items which fixes pdiff support combined
with a redirector and partial mirrors as in such a situation the pdiff
patches would be 404 and the complete index would be downloaded.
|
|
We have the last Release file around for other checks, so its trivial to
look if the new Release file contains a new codename (e.g. the user has
"testing" in the sources and it flipped from stretch to buster).
Such a change can be okay and expected, but also be a hint of problems,
so a warning if we see it happen seems okay. We can only print it once
anyhow and frontends and co are likely to ignore/hide it.
|
|
A suite or codename entry in the Release file is checked against the
distribution field in the sources.list entry that lead to the download of that
Release file. This distribution entry can contain slashes in the distribution
field:
deb http://security.debian.org/debian wheezy/updates main
However, the Release file may only contain "wheezy" in the Codename field and
not "wheezy/updates". So a transformation needs to take place that removes the
last / and everything that comes after (e.g. "/updates"). This fails, however,
for valid cases like a reprepro snapshot where the given Codename contains
slashes but is perfectly fine and doesn't need to be transformed. Since that
transformation is essentially just a workaround for special cases like the
security repository, it should be checked if the literal Codename without any
transformations happened is valid and only if isn't the dist should be checked
against the transformated one.
This way special cases like security.debian.org are handled and reprepro
snapshots work too.
The initial patch was taken as insperationto move whole transformation
to CheckDist() which makes this method more accepting & easier to use
(but according to codesearch.d.n we are the only users anyhow).
Thanks: Lukas Anzinger for initial patch
Closes: 644610
|
|
Some people do not recognize the field value with such an arcane name
and/or expect it to refer to something different (e.g. #839257).
We can't just rename it internally as its an avoidance strategy as such
fieldname existed previously with less clear semantics, but we can spare
the general public from this implementation detail.
|
|
A user relying on the deprecated behaviour of apt-get to accept a source
with an unknown pubkey to install a package containing the key expects
that the following 'apt-get update' causes the source to be considered
as trusted, but in case the source hadn't changed in the meantime this
wasn't happening: The source kept being untrusted until the Release file
was changed.
This only effects sources not using InRelease and only apt-get, the apt
binary downright refuses this course of actions, but it is a common way
of adding external sources.
Closes: 838779
|
|
Employ a priority queue instead of a normal queue to hold
the items; and only add items to the running pipeline if
their priority is the same or higher than the priority
of items in the queue.
The priorities are designed for a 3 stage pipeline system:
In stage 1, all Release files and .diff/Index files are fetched. This
allows us to determine what files remain to be fetched, and thus
ensures a usable progress reporting.
In stage 2, all Pdiff patches are fetched, so we can apply them
in parallel with fetching other files in stage 3.
In stage 3, all other files are fetched (complete index files
such as Contents, Packages).
Performance improvements, mainly from fetching the pdiff patches
before complete files, so they can be applied in parallel:
For the 01 Sep 2016 03:35:23 UTC -> 02 Sep 2016 09:25:37 update
of Debian unstable and testing with Contents and appstream for
amd64 and i386, update time reduced from 37 seconds to 24-28
seconds.
Previously, apt would first download new DEP11 icon tarballs
and metadata files, causing the CPU to be idle. By fetching
the diffs in stage 2, we can now patch our contents and Packages
files while we are downloading the DEP11 stuff.
|
|
Without randomizing the order in which we download the index files we
leak needlessly information to the mirrors of which architecture is
native or foreign on this system. More importantly, we leak the order in
which description translations will be used which in most cases will e.g.
have the native tongue first.
Note that the leak effect in practice is limited as apt detects if a file
it wants to download is already available in the latest version from a
previous download and does not query the server in such cases. Combined
with the fact that Translation files are usually updated infrequently
and not all at the same time, so a mirror can never be sure if it got asked
about all files the user wants.
|
|
|
|
This is needed on BSD where root's default group is wheel, not
root.
|
|
This fixes issues with chroots, but the goal here was to get
the test suite working on systems without dpkg.
|
|
This allows other vendors to use different paths, or to build
your own APT in /opt for testing. Note that this uses + 1 in
some places, as the paths we receive are absolute, but we need
to strip of the initial /.
|
|
The C.UTF-8 locale is not portable, so we need to use C, otherwise
we crash on other systems. We can use std::locale::classic() for
that, which might also be a bit cheaper than using locale("C").
|
|
The bugreport shows a segfault caused by the code not doing the correct
magical dance to remove an item from inside a queue in all cases. We
could try hard to fix this, but it is actually better and also easier to
perform these checks (which cause instant failure) earlier so that they
haven't entered queue(s) yet, which in return makes cleanup trivial.
The result is that we actually end up failing "too early" as if we
wouldn't be careful download errors would be logged before that process
was even started. Not a problem for the acquire system, but likely to
confuse users and programs alike if they see the download process
producing errors before apt was technically allowed to do an acquire
(it didn't, so no violation, but it looks like it to the untrained eye).
Closes: 835195
|
|
In af81ab9030229b4ce6cbe28f0f0831d4896fda01 by-hash got implemented as a
special compression type for our usual index files like Packages.
Missing in this scheme was the special .diff/Index index file containing
the info about individual patches for this index file. Deriving from the
index file class directly we inherent the compression handling
infrastructure and in this way also by-hash nearly for free.
Closes: #824926
|
|
The URI we later want to modify to get the file via by-hash was unset in
case a file was only available uncompressed (which is usually not the
case) causing an acquire error.
|
|
In af81ab9030229b4ce6cbe28f0f0831d4896fda01 we implement by-hash as a
special compression type, which breaks this filesize setting as the code
is looking for a foobar.by-hash file then. Dealing this slightly gets
us the intended value. Note that this has no direct effect as this value
will be set in other ways, too, and could only effect progress reporting.
Gbp-Dch: Ignore
|
|
If 9b8034a9fd40b4d05075fda719e61f6eb4c45678 serves the Release files
from a partial mirror we will end up getting 404 for some of the
indexes. Instead of giving up, we will instead ignore our same
redirection mirror constrain and ask the redirection service as a
potential hashsum mismatch is better than keeping the certain 404 error.
|
|
Now that we have the redirections loopchecker centrally in our items we
can use it also to prevent internal redirections to loop caused by
bugs as in a few instances we get into the business of rewriting the URI
we will query by ourself as we predict we would see such a redirect
anyway. Our code has no bugs of course, hence no practical difference. ;)
Gbp-Dch: Ignore
|
|
Having the detection handled in specific (http) workers means that a
redirection loop over different hostnames isn't detected. Its also not a
good idea have this implement in each method independently even if it
would work
|
|
Followup of b58e2c7c56b1416a343e81f9f80cb1f02c128e25.
Still a regression of sorts of 8b79c94af7f7cf2e5e5342294bc6e5a908cacabf.
Closes: 832044
|
